Re: [PATCH v1 1/5] mm/page_alloc: convert "report" flag of __free_one_page() to a proper flag

[David Hildenbrand]

On 02.10.20 15:41, Matthew Wilcox wrote:
> On Mon, Sep 28, 2020 at 08:21:06PM +0200, David Hildenbrand wrote:
>> Let's prepare for additional flags and avoid long parameter lists of bools.
>> Follow-up patches will also make use of the flags in __free_pages_ok(),
>> however, I wasn't able to come up with a better name for the type - should
>> be good enough for internal purposes.
> 
>> +/* Free One Page flags: for internal, non-pcp variants of free_pages(). */
>> +typedef int __bitwise fop_t;
> 
> That invites confusion with f_op.  There's no reason to use _t as a suffix
> here ... why not free_f?

git grep "bitwise" | grep typedef | grep include/linux

indicates that "_t" it the right thing to do.

I want a name that highlights that is is for the internal variants of
free_page(), free_f / free_t is too generic.

fpi_t (Free Page Internal) ?

> 
>> +/*
>> + * Skip free page reporting notification for the (possibly merged) page. (will
>> + * *not* mark the page reported, only skip the notification).
> 
> ... Don't you mean "will not skip marking the page as reported, only
> skip the notification"?

Yeah, I can use that.

The way free page reporting works is that

1. Free page reporting infrastructure will get notified after buddy
merging about a newly freed page.

2. Once a certain threshold of free pages is reached, it will pull pages
from the freelist, report them, and mark them as reported. (see
mm/page_reporting.c)

During 2., we didn't actually free a "new page", we only temporarily
removed it from the list, that's why we have to skip the notification.

What we do here is skip 1., not 2.

> 
> *reads code*
> 
> No, I'm still confused.  What does this sentence mean?
> 
> Would it help to have a FOP_DEFAULT that has FOP_REPORT_NOTIFY set and
> then a FOP_SKIP_REPORT_NOTIFY define that is 0?

Hmm, I'm not entirely sure if that improves the situation. Then, I need
3 defines instead of two, and an "inverse" documentation for
FOP_REPORT_NOTIFY.

> 
>> -static inline void __free_one_page(struct page *page,
>> -		unsigned long pfn,
>> -		struct zone *zone, unsigned int order,
>> -		int migratetype, bool report)
>> +static inline void __free_one_page(struct page *page, unsigned long pfn,
>> +				   struct zone *zone, unsigned int order,
>> +				   int migratetype, fop_t fop_flags)
> 
> Please don't over-indent like this.
> 
> static inline void __free_one_page(struct page *page, unsigned long pfn,
> 		struct zone *zone, unsigned int order, int migratetype,
> 		fop_t fop_flags)
> 
> reads just as well and then if someone needs to delete the 'static'
> later, they don't need to fiddle around with subsequent lines getting
> the whitespace to line up again.
> 

I don't care too much about this specific instance and can fix it up.
(this is clearly a matter of personal taste)

Thanks!

-- 
Thanks,

David / dhildenb