Messages in this thread | | | Subject | Re: [PATCH v1 1/5] mm/page_alloc: convert "report" flag of __free_one_page() to a proper flag | From | David Hildenbrand <> | Date | Fri, 2 Oct 2020 16:48:04 +0200 |
| |
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
| |