| Message ID | 20200120034252.1558-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-mm] mm/page_isolation: fix potential warning from user | expand |
Hello Qian, On 01/20/2020 09:12 AM, Qian Cai wrote: > It makes sense to call the WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE) > from the offlining path, but should avoid triggering it from userspace, > i.e, from is_mem_section_removable(). Could you elaborate why it makes sense not to warn about an unmovable ZONE_MOVABLE page when an user tries to query about a memory block device's movability through sysfs ? > > While at it, simplify the code a bit by removing an unnecessary jump > label and a local variable, so set_migratetype_isolate() could really > return a bool. > > Suggested-by: Michal Hocko <mhocko@kernel.org> > Signed-off-by: Qian Cai <cai@lca.pw> > --- > mm/page_alloc.c | 11 ++++------- > mm/page_isolation.c | 31 ++++++++++++++++++------------- > 2 files changed, 22 insertions(+), 20 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 621716a25639..3c4eb750a199 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8231,7 +8231,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > if (is_migrate_cma(migratetype)) > return NULL; > > - goto unmovable; > + return page; > } > > for (; iter < pageblock_nr_pages; iter++) { > @@ -8241,7 +8241,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > page = pfn_to_page(pfn + iter); > > if (PageReserved(page)) > - goto unmovable; > + return page; > > /* > * If the zone is movable and we have ruled out all reserved > @@ -8261,7 +8261,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > unsigned int skip_pages; > > if (!hugepage_migration_supported(page_hstate(head))) > - goto unmovable; > + return page; > > skip_pages = compound_nr(head) - (page - head); > iter += skip_pages - 1; > @@ -8303,12 +8303,9 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > * is set to both of a memory hole page and a _used_ kernel > * page at boot. > */ > - goto unmovable; > + return page; > } > return NULL; > -unmovable: > - WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE); > - return pfn_to_page(pfn + iter); > } > > #ifdef CONFIG_CONTIG_ALLOC > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index e70586523ca3..97f673d5fefa 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -15,12 +15,12 @@ > #define CREATE_TRACE_POINTS > #include <trace/events/page_isolation.h> > > -static int set_migratetype_isolate(struct page *page, int migratetype, int isol_flags) > +static bool set_migratetype_isolate(struct page *page, int migratetype, > + int isol_flags) > { > - struct page *unmovable = NULL; > + struct page *unmovable = ERR_PTR(-EBUSY); > struct zone *zone; > unsigned long flags; > - int ret = -EBUSY; > > zone = page_zone(page); > > @@ -49,21 +49,26 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_ > NULL); > > __mod_zone_freepage_state(zone, -nr_pages, mt); > - ret = 0; > } > > out: > spin_unlock_irqrestore(&zone->lock, flags); > - if (!ret) > + > + if (!unmovable) { > drain_all_pages(zone); > - else if ((isol_flags & REPORT_FAILURE) && unmovable) > - /* > - * printk() with zone->lock held will guarantee to trigger a > - * lockdep splat, so defer it here. > - */ > - dump_page(unmovable, "unmovable page"); > - > - return ret; > + } else { > + if (isol_flags & MEMORY_OFFLINE) > + WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);> + > + if ((isol_flags & REPORT_FAILURE) && !IS_ERR(unmovable)) > + /* > + * printk() with zone->lock held will likely trigger a > + * lockdep splat, so defer it here. > + */ > + dump_page(unmovable, "unmovable page"); > + } > + > + return !!unmovable; > } > > static void unset_migratetype_isolate(struct page *page, unsigned migratetype) set_migratetype_isolate() gets called from CMA as well as HugeTLB allocation paths, so its not only during offline. Hence the commit message should be changed to reflect this. - Anshuman >
On Mon 20-01-20 09:50:33, Anshuman Khandual wrote: > Hello Qian, > > On 01/20/2020 09:12 AM, Qian Cai wrote: > > It makes sense to call the WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE) > > from the offlining path, but should avoid triggering it from userspace, > > i.e, from is_mem_section_removable(). > > Could you elaborate why it makes sense not to warn about an unmovable > ZONE_MOVABLE page when an user tries to query about a memory block > device's movability through sysfs ? Because somebody might have panic_on_warn and then this is unlikely (but not impossible) way to put the system down by arbitrary user. Besides that it is stupid to warn when we convey the information to the userspace anyway. [...] > > + } else { > > + if (isol_flags & MEMORY_OFFLINE) > > + WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);> + > > + if ((isol_flags & REPORT_FAILURE) && !IS_ERR(unmovable)) > > + /* > > + * printk() with zone->lock held will likely trigger a > > + * lockdep splat, so defer it here. > > + */ > > + dump_page(unmovable, "unmovable page"); > > + } > > + > > + return !!unmovable; > > } > > > > static void unset_migratetype_isolate(struct page *page, unsigned migratetype) > > set_migratetype_isolate() gets called from CMA as well as HugeTLB > allocation paths, so its not only during offline. Hence the commit > message should be changed to reflect this. We should just report for all those cases I believe.
>>> + } else { >>> + if (isol_flags & MEMORY_OFFLINE) >>> + WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);> + >>> + if ((isol_flags & REPORT_FAILURE) && !IS_ERR(unmovable)) >>> + /* >>> + * printk() with zone->lock held will likely trigger a >>> + * lockdep splat, so defer it here. >>> + */ >>> + dump_page(unmovable, "unmovable page"); >>> + } >>> + >>> + return !!unmovable; >>> } >>> >>> static void unset_migratetype_isolate(struct page *page, unsigned migratetype) >> >> set_migratetype_isolate() gets called from CMA as well as HugeTLB >> allocation paths, so its not only during offline. Hence the commit >> message should be changed to reflect this. > > We should just report for all those cases I believe. > +1
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 621716a25639..3c4eb750a199 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8231,7 +8231,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, if (is_migrate_cma(migratetype)) return NULL; - goto unmovable; + return page; } for (; iter < pageblock_nr_pages; iter++) { @@ -8241,7 +8241,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, page = pfn_to_page(pfn + iter); if (PageReserved(page)) - goto unmovable; + return page; /* * If the zone is movable and we have ruled out all reserved @@ -8261,7 +8261,7 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, unsigned int skip_pages; if (!hugepage_migration_supported(page_hstate(head))) - goto unmovable; + return page; skip_pages = compound_nr(head) - (page - head); iter += skip_pages - 1; @@ -8303,12 +8303,9 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, * is set to both of a memory hole page and a _used_ kernel * page at boot. */ - goto unmovable; + return page; } return NULL; -unmovable: - WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE); - return pfn_to_page(pfn + iter); } #ifdef CONFIG_CONTIG_ALLOC diff --git a/mm/page_isolation.c b/mm/page_isolation.c index e70586523ca3..97f673d5fefa 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -15,12 +15,12 @@ #define CREATE_TRACE_POINTS #include <trace/events/page_isolation.h> -static int set_migratetype_isolate(struct page *page, int migratetype, int isol_flags) +static bool set_migratetype_isolate(struct page *page, int migratetype, + int isol_flags) { - struct page *unmovable = NULL; + struct page *unmovable = ERR_PTR(-EBUSY); struct zone *zone; unsigned long flags; - int ret = -EBUSY; zone = page_zone(page); @@ -49,21 +49,26 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_ NULL); __mod_zone_freepage_state(zone, -nr_pages, mt); - ret = 0; } out: spin_unlock_irqrestore(&zone->lock, flags); - if (!ret) + + if (!unmovable) { drain_all_pages(zone); - else if ((isol_flags & REPORT_FAILURE) && unmovable) - /* - * printk() with zone->lock held will guarantee to trigger a - * lockdep splat, so defer it here. - */ - dump_page(unmovable, "unmovable page"); - - return ret; + } else { + if (isol_flags & MEMORY_OFFLINE) + WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE); + + if ((isol_flags & REPORT_FAILURE) && !IS_ERR(unmovable)) + /* + * printk() with zone->lock held will likely trigger a + * lockdep splat, so defer it here. + */ + dump_page(unmovable, "unmovable page"); + } + + return !!unmovable; } static void unset_migratetype_isolate(struct page *page, unsigned migratetype)
It makes sense to call the WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE) from the offlining path, but should avoid triggering it from userspace, i.e, from is_mem_section_removable(). While at it, simplify the code a bit by removing an unnecessary jump label and a local variable, so set_migratetype_isolate() could really return a bool. Suggested-by: Michal Hocko <mhocko@kernel.org> Signed-off-by: Qian Cai <cai@lca.pw> --- mm/page_alloc.c | 11 ++++------- mm/page_isolation.c | 31 ++++++++++++++++++------------- 2 files changed, 22 insertions(+), 20 deletions(-)