On Thu 18-03-21 12:10:14, Vlastimil Babka wrote:

On 3/18/21 11:22 AM, Michal Hocko wrote:

[...]

quoted

E.g. something like the following

diff --git a/mm/internal.h b/mm/internal.h
index 1432feec62df..6c5a9066adf0 100644
--- a/mm/internal.h
+++ b/mm/internal.h

@@ -225,7 +225,13 @@ struct compact_control {
 	unsigned int nr_freepages;	/* Number of isolated free pages */
 	unsigned int nr_migratepages;	/* Number of pages to migrate */
 	unsigned long free_pfn;		/* isolate_freepages search base */
-	unsigned long migrate_pfn;	/* isolate_migratepages search base */
+	unsigned long migrate_pfn;	/* Acts as an in/out parameter to page
+					 * isolation.
+					 * isolate_migratepages uses it as a search base.
+					 * isolate_migratepages_block will update the
+					 * value the next pfn after the last isolated
+					 * one.
+					 */

Fair enough. I would even stop pretending we might cram something useful in the
rest of the line, and move all the comments to blocks before the variables.
There might be more of them that would deserve more thorough description.

Yeah, makes sense. I am not a fan of the above form of documentation.
Btw. maybe renaming the field would be even better, both from the
intention and review all existing users. I would go with pfn_iter or
something that wouldn't make it sound like migration specific.

-- 
Michal Hocko
SUSE Labs