| Message ID | 20200928182110.7050-2-david@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/5] mm/page_alloc: convert "report" flag of __free_one_page() to a proper flag | expand |
> 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. > > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > Reviewed-by: Oscar Salvador <osalvador@suse.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> > Cc: Mel Gorman <mgorman@techsingularity.net> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Wei Yang <richard.weiyang@linux.alibaba.com> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Mike Rapoport <rppt@kernel.org> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/page_alloc.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index df90e3654f97..daab90e960fe 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -77,6 +77,18 @@ > #include "shuffle.h" > #include "page_reporting.h" > > +/* Free One Page flags: for internal, non-pcp variants of free_pages(). */ > +typedef int __bitwise fop_t; > + > +/* No special request */ > +#define FOP_NONE ((__force fop_t)0) > + > +/* > + * Skip free page reporting notification for the (possibly merged) page. (will > + * *not* mark the page reported, only skip the notification). > + */ > +#define FOP_SKIP_REPORT_NOTIFY ((__force fop_t)BIT(0)) > + > /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */ > static DEFINE_MUTEX(pcp_batch_high_lock); > #define MIN_PERCPU_PAGELIST_FRACTION (8) > @@ -948,10 +960,9 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, > * -- nyc > */ > > -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) > { > struct capture_control *capc = task_capc(zone); > unsigned long buddy_pfn; > @@ -1038,7 +1049,7 @@ static inline void __free_one_page(struct page *page, > add_to_free_list(page, zone, order, migratetype); > > /* Notify page reporting subsystem of freed page */ > - if (report) > + if (!(fop_flags & FOP_SKIP_REPORT_NOTIFY)) > page_reporting_notify_free(order); > } > > @@ -1379,7 +1390,7 @@ static void free_pcppages_bulk(struct zone *zone, int count, > if (unlikely(isolated_pageblocks)) > mt = get_pageblock_migratetype(page); > > - __free_one_page(page, page_to_pfn(page), zone, 0, mt, true); > + __free_one_page(page, page_to_pfn(page), zone, 0, mt, FOP_NONE); > trace_mm_page_pcpu_drain(page, 0, mt); > } > spin_unlock(&zone->lock); > @@ -1395,7 +1406,7 @@ static void free_one_page(struct zone *zone, > is_migrate_isolate(migratetype))) { > migratetype = get_pfnblock_migratetype(page, pfn); > } > - __free_one_page(page, pfn, zone, order, migratetype, true); > + __free_one_page(page, pfn, zone, order, migratetype, FOP_NONE); > spin_unlock(&zone->lock); > } > > @@ -3288,7 +3299,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt) > lockdep_assert_held(&zone->lock); > > /* Return isolated page to tail of freelist. */ > - __free_one_page(page, page_to_pfn(page), zone, order, mt, false); > + __free_one_page(page, page_to_pfn(page), zone, order, mt, > + FOP_SKIP_REPORT_NOTIFY); > } Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
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. > >Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >Reviewed-by: Oscar Salvador <osalvador@suse.de> >Cc: Andrew Morton <akpm@linux-foundation.org> >Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> >Cc: Mel Gorman <mgorman@techsingularity.net> >Cc: Michal Hocko <mhocko@kernel.org> >Cc: Dave Hansen <dave.hansen@intel.com> >Cc: Vlastimil Babka <vbabka@suse.cz> >Cc: Wei Yang <richard.weiyang@linux.alibaba.com> >Cc: Oscar Salvador <osalvador@suse.de> >Cc: Mike Rapoport <rppt@kernel.org> >Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Wei Yang <richard.weiyang@linux.alibaba.com> >--- > mm/page_alloc.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > >diff --git a/mm/page_alloc.c b/mm/page_alloc.c >index df90e3654f97..daab90e960fe 100644 >--- a/mm/page_alloc.c >+++ b/mm/page_alloc.c >@@ -77,6 +77,18 @@ > #include "shuffle.h" > #include "page_reporting.h" > >+/* Free One Page flags: for internal, non-pcp variants of free_pages(). */ >+typedef int __bitwise fop_t; >+ >+/* No special request */ >+#define FOP_NONE ((__force fop_t)0) >+ >+/* >+ * Skip free page reporting notification for the (possibly merged) page. (will >+ * *not* mark the page reported, only skip the notification). >+ */ >+#define FOP_SKIP_REPORT_NOTIFY ((__force fop_t)BIT(0)) >+ > /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */ > static DEFINE_MUTEX(pcp_batch_high_lock); > #define MIN_PERCPU_PAGELIST_FRACTION (8) >@@ -948,10 +960,9 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, > * -- nyc > */ > >-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) > { > struct capture_control *capc = task_capc(zone); > unsigned long buddy_pfn; >@@ -1038,7 +1049,7 @@ static inline void __free_one_page(struct page *page, > add_to_free_list(page, zone, order, migratetype); > > /* Notify page reporting subsystem of freed page */ >- if (report) >+ if (!(fop_flags & FOP_SKIP_REPORT_NOTIFY)) > page_reporting_notify_free(order); > } > >@@ -1379,7 +1390,7 @@ static void free_pcppages_bulk(struct zone *zone, int count, > if (unlikely(isolated_pageblocks)) > mt = get_pageblock_migratetype(page); > >- __free_one_page(page, page_to_pfn(page), zone, 0, mt, true); >+ __free_one_page(page, page_to_pfn(page), zone, 0, mt, FOP_NONE); > trace_mm_page_pcpu_drain(page, 0, mt); > } > spin_unlock(&zone->lock); >@@ -1395,7 +1406,7 @@ static void free_one_page(struct zone *zone, > is_migrate_isolate(migratetype))) { > migratetype = get_pfnblock_migratetype(page, pfn); > } >- __free_one_page(page, pfn, zone, order, migratetype, true); >+ __free_one_page(page, pfn, zone, order, migratetype, FOP_NONE); > spin_unlock(&zone->lock); > } > >@@ -3288,7 +3299,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt) > lockdep_assert_held(&zone->lock); > > /* Return isolated page to tail of freelist. */ >- __free_one_page(page, page_to_pfn(page), zone, order, mt, false); >+ __free_one_page(page, page_to_pfn(page), zone, order, mt, >+ FOP_SKIP_REPORT_NOTIFY); > } > > /* >-- >2.26.2
On Mon 28-09-20 20:21:06, 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. > > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > Reviewed-by: Oscar Salvador <osalvador@suse.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> > Cc: Mel Gorman <mgorman@techsingularity.net> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Wei Yang <richard.weiyang@linux.alibaba.com> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Mike Rapoport <rppt@kernel.org> > Signed-off-by: David Hildenbrand <david@redhat.com> Hopefully this will not wrack the generated code. But considering we would need another parameter there is not much choice left. Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/page_alloc.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index df90e3654f97..daab90e960fe 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -77,6 +77,18 @@ > #include "shuffle.h" > #include "page_reporting.h" > > +/* Free One Page flags: for internal, non-pcp variants of free_pages(). */ > +typedef int __bitwise fop_t; > + > +/* No special request */ > +#define FOP_NONE ((__force fop_t)0) > + > +/* > + * Skip free page reporting notification for the (possibly merged) page. (will > + * *not* mark the page reported, only skip the notification). > + */ > +#define FOP_SKIP_REPORT_NOTIFY ((__force fop_t)BIT(0)) > + > /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */ > static DEFINE_MUTEX(pcp_batch_high_lock); > #define MIN_PERCPU_PAGELIST_FRACTION (8) > @@ -948,10 +960,9 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, > * -- nyc > */ > > -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) > { > struct capture_control *capc = task_capc(zone); > unsigned long buddy_pfn; > @@ -1038,7 +1049,7 @@ static inline void __free_one_page(struct page *page, > add_to_free_list(page, zone, order, migratetype); > > /* Notify page reporting subsystem of freed page */ > - if (report) > + if (!(fop_flags & FOP_SKIP_REPORT_NOTIFY)) > page_reporting_notify_free(order); > } > > @@ -1379,7 +1390,7 @@ static void free_pcppages_bulk(struct zone *zone, int count, > if (unlikely(isolated_pageblocks)) > mt = get_pageblock_migratetype(page); > > - __free_one_page(page, page_to_pfn(page), zone, 0, mt, true); > + __free_one_page(page, page_to_pfn(page), zone, 0, mt, FOP_NONE); > trace_mm_page_pcpu_drain(page, 0, mt); > } > spin_unlock(&zone->lock); > @@ -1395,7 +1406,7 @@ static void free_one_page(struct zone *zone, > is_migrate_isolate(migratetype))) { > migratetype = get_pfnblock_migratetype(page, pfn); > } > - __free_one_page(page, pfn, zone, order, migratetype, true); > + __free_one_page(page, pfn, zone, order, migratetype, FOP_NONE); > spin_unlock(&zone->lock); > } > > @@ -3288,7 +3299,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt) > lockdep_assert_held(&zone->lock); > > /* Return isolated page to tail of freelist. */ > - __free_one_page(page, page_to_pfn(page), zone, order, mt, false); > + __free_one_page(page, page_to_pfn(page), zone, order, mt, > + FOP_SKIP_REPORT_NOTIFY); > } > > /* > -- > 2.26.2
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? > +/* > + * 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"? *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? > -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.
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
On 02.10.20 16:48, David Hildenbrand wrote: > 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. Reading again, it doesn't quite fit. Marking pages as reported is handled by mm/page_reporting.c /* * Skip free page reporting notification for the (possibly merged) page. * This does not hinder free page reporting from grabbing the page, * reporting it and marking it "reported" - it only skips notifying * the free page reporting infrastructure about a newly freed page. For * example, used when temporarily pulling a page from the freelist and * putting it back unmodified. */ Is that clearer?
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index df90e3654f97..daab90e960fe 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -77,6 +77,18 @@ #include "shuffle.h" #include "page_reporting.h" +/* Free One Page flags: for internal, non-pcp variants of free_pages(). */ +typedef int __bitwise fop_t; + +/* No special request */ +#define FOP_NONE ((__force fop_t)0) + +/* + * Skip free page reporting notification for the (possibly merged) page. (will + * *not* mark the page reported, only skip the notification). + */ +#define FOP_SKIP_REPORT_NOTIFY ((__force fop_t)BIT(0)) + /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */ static DEFINE_MUTEX(pcp_batch_high_lock); #define MIN_PERCPU_PAGELIST_FRACTION (8) @@ -948,10 +960,9 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn, * -- nyc */ -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) { struct capture_control *capc = task_capc(zone); unsigned long buddy_pfn; @@ -1038,7 +1049,7 @@ static inline void __free_one_page(struct page *page, add_to_free_list(page, zone, order, migratetype); /* Notify page reporting subsystem of freed page */ - if (report) + if (!(fop_flags & FOP_SKIP_REPORT_NOTIFY)) page_reporting_notify_free(order); } @@ -1379,7 +1390,7 @@ static void free_pcppages_bulk(struct zone *zone, int count, if (unlikely(isolated_pageblocks)) mt = get_pageblock_migratetype(page); - __free_one_page(page, page_to_pfn(page), zone, 0, mt, true); + __free_one_page(page, page_to_pfn(page), zone, 0, mt, FOP_NONE); trace_mm_page_pcpu_drain(page, 0, mt); } spin_unlock(&zone->lock); @@ -1395,7 +1406,7 @@ static void free_one_page(struct zone *zone, is_migrate_isolate(migratetype))) { migratetype = get_pfnblock_migratetype(page, pfn); } - __free_one_page(page, pfn, zone, order, migratetype, true); + __free_one_page(page, pfn, zone, order, migratetype, FOP_NONE); spin_unlock(&zone->lock); } @@ -3288,7 +3299,8 @@ void __putback_isolated_page(struct page *page, unsigned int order, int mt) lockdep_assert_held(&zone->lock); /* Return isolated page to tail of freelist. */ - __free_one_page(page, page_to_pfn(page), zone, order, mt, false); + __free_one_page(page, page_to_pfn(page), zone, order, mt, + FOP_SKIP_REPORT_NOTIFY); } /*