* [PATCH -mm] mm/page_isolation: fix potential warning from user
@ 2020-01-20 3:42 Qian Cai
2020-01-20 4:20 ` Anshuman Khandual
0 siblings, 1 reply; 4+ messages in thread
From: Qian Cai @ 2020-01-20 3:42 UTC (permalink / raw)
To: akpm; +Cc: mhocko, david, linux-mm, linux-kernel, Qian Cai
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(-)
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)
--
2.21.0 (Apple Git-122.2)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH -mm] mm/page_isolation: fix potential warning from user
2020-01-20 3:42 [PATCH -mm] mm/page_isolation: fix potential warning from user Qian Cai
@ 2020-01-20 4:20 ` Anshuman Khandual
2020-01-20 7:42 ` Michal Hocko
0 siblings, 1 reply; 4+ messages in thread
From: Anshuman Khandual @ 2020-01-20 4:20 UTC (permalink / raw)
To: Qian Cai, akpm; +Cc: mhocko, david, linux-mm, linux-kernel
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -mm] mm/page_isolation: fix potential warning from user
2020-01-20 4:20 ` Anshuman Khandual
@ 2020-01-20 7:42 ` Michal Hocko
2020-01-20 9:42 ` David Hildenbrand
0 siblings, 1 reply; 4+ messages in thread
From: Michal Hocko @ 2020-01-20 7:42 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: Qian Cai, akpm, david, linux-mm, linux-kernel
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.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -mm] mm/page_isolation: fix potential warning from user
2020-01-20 7:42 ` Michal Hocko
@ 2020-01-20 9:42 ` David Hildenbrand
0 siblings, 0 replies; 4+ messages in thread
From: David Hildenbrand @ 2020-01-20 9:42 UTC (permalink / raw)
To: Michal Hocko, Anshuman Khandual; +Cc: Qian Cai, akpm, linux-mm, linux-kernel
>>> + } 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
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-20 9:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 3:42 [PATCH -mm] mm/page_isolation: fix potential warning from user Qian Cai
2020-01-20 4:20 ` Anshuman Khandual
2020-01-20 7:42 ` Michal Hocko
2020-01-20 9:42 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).