| Message ID | 20190907172528.10910.37051.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stg mail -e --version=v9 \ | expand |
On 07.09.19 19:25, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > In order to support page reporting it will be necessary to store and > retrieve the migratetype of a page. To enable that I am moving the set and > get operations for pcppage_migratetype into the mm/internal.h header so > that they can be used outside of the page_alloc.c file. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > --- > mm/internal.h | 18 ++++++++++++++++++ > mm/page_alloc.c | 18 ------------------ > 2 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/mm/internal.h b/mm/internal.h > index 0d5f720c75ab..e4a1a57bbd40 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -549,6 +549,24 @@ static inline bool is_migrate_highatomic_page(struct page *page) > return get_pageblock_migratetype(page) == MIGRATE_HIGHATOMIC; > } > > +/* > + * A cached value of the page's pageblock's migratetype, used when the page is > + * put on a pcplist. Used to avoid the pageblock migratetype lookup when > + * freeing from pcplists in most cases, at the cost of possibly becoming stale. > + * Also the migratetype set in the page does not necessarily match the pcplist > + * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any > + * other index - this ensures that it will be put on the correct CMA freelist. > + */ > +static inline int get_pcppage_migratetype(struct page *page) > +{ > + return page->index; > +} > + > +static inline void set_pcppage_migratetype(struct page *page, int migratetype) > +{ > + page->index = migratetype; > +} > + > void setup_zone_pageset(struct zone *zone); > extern struct page *alloc_new_node_page(struct page *page, unsigned long node); > #endif /* __MM_INTERNAL_H */ > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 4e4356ba66c7..a791f2baeeeb 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -185,24 +185,6 @@ static int __init early_init_on_free(char *buf) > } > early_param("init_on_free", early_init_on_free); > > -/* > - * A cached value of the page's pageblock's migratetype, used when the page is > - * put on a pcplist. Used to avoid the pageblock migratetype lookup when > - * freeing from pcplists in most cases, at the cost of possibly becoming stale. > - * Also the migratetype set in the page does not necessarily match the pcplist > - * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any > - * other index - this ensures that it will be put on the correct CMA freelist. > - */ > -static inline int get_pcppage_migratetype(struct page *page) > -{ > - return page->index; > -} > - > -static inline void set_pcppage_migratetype(struct page *page, int migratetype) > -{ > - page->index = migratetype; > -} > - > #ifdef CONFIG_PM_SLEEP > /* > * The following functions are used by the suspend/hibernate code to temporarily > > Still have to understand in detail how this will be used, but the change certainly looks ok :) Acked-by: David Hildenbrand <david@redhat.com>
On Sat, Sep 07, 2019 at 10:25:28AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > In order to support page reporting it will be necessary to store and > retrieve the migratetype of a page. To enable that I am moving the set and > get operations for pcppage_migratetype into the mm/internal.h header so > that they can be used outside of the page_alloc.c file. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> I'm not sure that it's great idea to export this functionality beyond mm/page_alloc.c without any additional safeguards. How would we avoid to messing with ->index when the page is not in the right state of its life-cycle. Can we add some VM_BUG_ON()s here?
On Mon, 2019-09-09 at 12:56 +0300, Kirill A. Shutemov wrote: > On Sat, Sep 07, 2019 at 10:25:28AM -0700, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > In order to support page reporting it will be necessary to store and > > retrieve the migratetype of a page. To enable that I am moving the set and > > get operations for pcppage_migratetype into the mm/internal.h header so > > that they can be used outside of the page_alloc.c file. > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > I'm not sure that it's great idea to export this functionality beyond > mm/page_alloc.c without any additional safeguards. How would we avoid to > messing with ->index when the page is not in the right state of its > life-cycle. Can we add some VM_BUG_ON()s here? I am not sure what we would need to check on though. There are essentially 2 cases where we are using this. The first is the percpu page lists so the value is set either as a result of __rmqueue_smallest or free_unref_page_prepare. The second one which hasn't been added yet is for the Reported pages case which I add with patch 6. When I use it for page reporting I am essentially using the Reported flag to identify what pages in the buddy list will have this value set versus those that may not. I didn't explicitly define it that way, but that is how I am using it so that the value cannot be trusted unless the Reported flag is set.
On Mon, 2019-09-09 at 11:01 -0700, Alexander Duyck wrote: > On Mon, 2019-09-09 at 12:56 +0300, Kirill A. Shutemov wrote: > > On Sat, Sep 07, 2019 at 10:25:28AM -0700, Alexander Duyck wrote: > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > In order to support page reporting it will be necessary to store and > > > retrieve the migratetype of a page. To enable that I am moving the set and > > > get operations for pcppage_migratetype into the mm/internal.h header so > > > that they can be used outside of the page_alloc.c file. > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > I'm not sure that it's great idea to export this functionality beyond > > mm/page_alloc.c without any additional safeguards. How would we avoid to > > messing with ->index when the page is not in the right state of its > > life-cycle. Can we add some VM_BUG_ON()s here? > > I am not sure what we would need to check on though. There are essentially > 2 cases where we are using this. The first is the percpu page lists so the > value is set either as a result of __rmqueue_smallest or > free_unref_page_prepare. The second one which hasn't been added yet is for > the Reported pages case which I add with patch 6. > > When I use it for page reporting I am essentially using the Reported flag > to identify what pages in the buddy list will have this value set versus > those that may not. I didn't explicitly define it that way, but that is > how I am using it so that the value cannot be trusted unless the Reported > flag is set. I guess the alternative would be to just treat the ->index value as the index within the boundary array, and not use the per-cpu list functions. Doing that might make things a bit more clear since all we are really doing is storing the index into the boundary list the page is contained in. I could probably combine the value of order and migratetype and save myself a few cycles in the process by just saving the index into the array directly.
On Sat 07-09-19 10:25:28, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > In order to support page reporting it will be necessary to store and > retrieve the migratetype of a page. To enable that I am moving the set and > get operations for pcppage_migratetype into the mm/internal.h header so > that they can be used outside of the page_alloc.c file. Please describe who is the user and why does it needs this interface. This is really important because migratetype is an MM internal thing and external users shouldn't really care about it at all. We really do not want a random code to call those, especially the set_pcppage_migratetype. > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > --- > mm/internal.h | 18 ++++++++++++++++++ > mm/page_alloc.c | 18 ------------------ > 2 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/mm/internal.h b/mm/internal.h > index 0d5f720c75ab..e4a1a57bbd40 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -549,6 +549,24 @@ static inline bool is_migrate_highatomic_page(struct page *page) > return get_pageblock_migratetype(page) == MIGRATE_HIGHATOMIC; > } > > +/* > + * A cached value of the page's pageblock's migratetype, used when the page is > + * put on a pcplist. Used to avoid the pageblock migratetype lookup when > + * freeing from pcplists in most cases, at the cost of possibly becoming stale. > + * Also the migratetype set in the page does not necessarily match the pcplist > + * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any > + * other index - this ensures that it will be put on the correct CMA freelist. > + */ > +static inline int get_pcppage_migratetype(struct page *page) > +{ > + return page->index; > +} > + > +static inline void set_pcppage_migratetype(struct page *page, int migratetype) > +{ > + page->index = migratetype; > +} > + > void setup_zone_pageset(struct zone *zone); > extern struct page *alloc_new_node_page(struct page *page, unsigned long node); > #endif /* __MM_INTERNAL_H */ > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 4e4356ba66c7..a791f2baeeeb 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -185,24 +185,6 @@ static int __init early_init_on_free(char *buf) > } > early_param("init_on_free", early_init_on_free); > > -/* > - * A cached value of the page's pageblock's migratetype, used when the page is > - * put on a pcplist. Used to avoid the pageblock migratetype lookup when > - * freeing from pcplists in most cases, at the cost of possibly becoming stale. > - * Also the migratetype set in the page does not necessarily match the pcplist > - * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any > - * other index - this ensures that it will be put on the correct CMA freelist. > - */ > -static inline int get_pcppage_migratetype(struct page *page) > -{ > - return page->index; > -} > - > -static inline void set_pcppage_migratetype(struct page *page, int migratetype) > -{ > - page->index = migratetype; > -} > - > #ifdef CONFIG_PM_SLEEP > /* > * The following functions are used by the suspend/hibernate code to temporarily
On Tue, Sep 10, 2019 at 5:23 AM Michal Hocko <mhocko@kernel.org> wrote: > > On Sat 07-09-19 10:25:28, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > In order to support page reporting it will be necessary to store and > > retrieve the migratetype of a page. To enable that I am moving the set and > > get operations for pcppage_migratetype into the mm/internal.h header so > > that they can be used outside of the page_alloc.c file. > > Please describe who is the user and why does it needs this interface. > This is really important because migratetype is an MM internal thing and > external users shouldn't really care about it at all. We really do not > want a random code to call those, especially the set_pcppage_migratetype. I was using it to store the migratetype of the page so that I could find the boundary list that contained the reported page as the array is indexed based on page order and migratetype. However on further discussion I am thinking I may just use page->index directly to index into the boundary array. Doing that I should be able to get a very slight improvement in lookup time since I am not having to pull order and migratetype and then compute the index based on that. In addition it becomes much more clear as to what is going on, and if needed I could add debug checks to verify the page is "Reported" and that the "Buddy" page type is set.
On Tue 10-09-19 07:46:50, Alexander Duyck wrote: > On Tue, Sep 10, 2019 at 5:23 AM Michal Hocko <mhocko@kernel.org> wrote: > > > > On Sat 07-09-19 10:25:28, Alexander Duyck wrote: > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > In order to support page reporting it will be necessary to store and > > > retrieve the migratetype of a page. To enable that I am moving the set and > > > get operations for pcppage_migratetype into the mm/internal.h header so > > > that they can be used outside of the page_alloc.c file. > > > > Please describe who is the user and why does it needs this interface. > > This is really important because migratetype is an MM internal thing and > > external users shouldn't really care about it at all. We really do not > > want a random code to call those, especially the set_pcppage_migratetype. > > I was using it to store the migratetype of the page so that I could > find the boundary list that contained the reported page as the array > is indexed based on page order and migratetype. However on further > discussion I am thinking I may just use page->index directly to index > into the boundary array. Doing that I should be able to get a very > slight improvement in lookup time since I am not having to pull order > and migratetype and then compute the index based on that. In addition > it becomes much more clear as to what is going on, and if needed I > could add debug checks to verify the page is "Reported" and that the > "Buddy" page type is set. Be careful though. A free page belongs to the page allocator and it is free to reuse any fields for its purpose so using any of them nilly willy is no go. If you need to stuff something like that then there better be an api the allocator is aware of. My main objection is the abuse migrate type. There might be other ways to express what you need. Please make sure you clearly define that though.
On Tue, 2019-09-10 at 19:45 +0200, Michal Hocko wrote: > On Tue 10-09-19 07:46:50, Alexander Duyck wrote: > > On Tue, Sep 10, 2019 at 5:23 AM Michal Hocko <mhocko@kernel.org> wrote: > > > On Sat 07-09-19 10:25:28, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > > > > > In order to support page reporting it will be necessary to store and > > > > retrieve the migratetype of a page. To enable that I am moving the set and > > > > get operations for pcppage_migratetype into the mm/internal.h header so > > > > that they can be used outside of the page_alloc.c file. > > > > > > Please describe who is the user and why does it needs this interface. > > > This is really important because migratetype is an MM internal thing and > > > external users shouldn't really care about it at all. We really do not > > > want a random code to call those, especially the set_pcppage_migratetype. > > > > I was using it to store the migratetype of the page so that I could > > find the boundary list that contained the reported page as the array > > is indexed based on page order and migratetype. However on further > > discussion I am thinking I may just use page->index directly to index > > into the boundary array. Doing that I should be able to get a very > > slight improvement in lookup time since I am not having to pull order > > and migratetype and then compute the index based on that. In addition > > it becomes much more clear as to what is going on, and if needed I > > could add debug checks to verify the page is "Reported" and that the > > "Buddy" page type is set. > > Be careful though. A free page belongs to the page allocator and it is > free to reuse any fields for its purpose so using any of them nilly > willy is no go. If you need to stuff something like that then there > better be an api the allocator is aware of. My main objection is the > abuse migrate type. There might be other ways to express what you need. > Please make sure you clearly define that though. I will. Basically if the Reported is set then it will mean that the index value is in use and provides the index into the boundary array. The Reported flag will be cleared when the page is pulled from the buddy list and in the case of the page being allocated it is already overwritten by __rmqueue_smallest calling set_pcppage_migratetype which is what gave me the idea to just use that in the first place.
diff --git a/mm/internal.h b/mm/internal.h index 0d5f720c75ab..e4a1a57bbd40 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -549,6 +549,24 @@ static inline bool is_migrate_highatomic_page(struct page *page) return get_pageblock_migratetype(page) == MIGRATE_HIGHATOMIC; } +/* + * A cached value of the page's pageblock's migratetype, used when the page is + * put on a pcplist. Used to avoid the pageblock migratetype lookup when + * freeing from pcplists in most cases, at the cost of possibly becoming stale. + * Also the migratetype set in the page does not necessarily match the pcplist + * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any + * other index - this ensures that it will be put on the correct CMA freelist. + */ +static inline int get_pcppage_migratetype(struct page *page) +{ + return page->index; +} + +static inline void set_pcppage_migratetype(struct page *page, int migratetype) +{ + page->index = migratetype; +} + void setup_zone_pageset(struct zone *zone); extern struct page *alloc_new_node_page(struct page *page, unsigned long node); #endif /* __MM_INTERNAL_H */ diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 4e4356ba66c7..a791f2baeeeb 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -185,24 +185,6 @@ static int __init early_init_on_free(char *buf) } early_param("init_on_free", early_init_on_free); -/* - * A cached value of the page's pageblock's migratetype, used when the page is - * put on a pcplist. Used to avoid the pageblock migratetype lookup when - * freeing from pcplists in most cases, at the cost of possibly becoming stale. - * Also the migratetype set in the page does not necessarily match the pcplist - * index, e.g. page might have MIGRATE_CMA set but be on a pcplist with any - * other index - this ensures that it will be put on the correct CMA freelist. - */ -static inline int get_pcppage_migratetype(struct page *page) -{ - return page->index; -} - -static inline void set_pcppage_migratetype(struct page *page, int migratetype) -{ - page->index = migratetype; -} - #ifdef CONFIG_PM_SLEEP /* * The following functions are used by the suspend/hibernate code to temporarily