Having unmovable pages on a given pageblock should be reported correctly
when required with REPORT_FAILURE flag. But there can be a scenario where a
reserved page in the page block will get reported as a generic "unmovable"
reason code. Instead this should be changed to a more appropriate reason
code like "Reserved page".
Cc: Andrew Morton <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Oscar Salvador <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Pavel Tatashin <[email protected]>
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
mm/page_alloc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 15c2050c629b..fbf93ea119d2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8206,8 +8206,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
page = pfn_to_page(check);
- if (PageReserved(page))
+ if (PageReserved(page)) {
+ reason = "Reserved page";
goto unmovable;
+ }
/*
* If the zone is movable and we have ruled out all reserved
--
2.20.1
> On Oct 3, 2019, at 4:10 AM, Anshuman Khandual <[email protected]> wrote:
>
> Having unmovable pages on a given pageblock should be reported correctly
> when required with REPORT_FAILURE flag. But there can be a scenario where a
> reserved page in the page block will get reported as a generic "unmovable"
> reason code. Instead this should be changed to a more appropriate reason
> code like "Reserved page".
Isn’t this redundant as it dumps the flags in dump_page() anyway?
>
> Cc: Andrew Morton <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Oscar Salvador <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Mike Rapoport <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Pavel Tatashin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> mm/page_alloc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 15c2050c629b..fbf93ea119d2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8206,8 +8206,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>
> page = pfn_to_page(check);
>
> - if (PageReserved(page))
> + if (PageReserved(page)) {
> + reason = "Reserved page";
> goto unmovable;
> + }
>
> /*
> * If the zone is movable and we have ruled out all reserved
> --
> 2.20.1
>
On 10/03/2019 02:35 PM, Qian Cai wrote:
>
>
>> On Oct 3, 2019, at 4:10 AM, Anshuman Khandual <[email protected]> wrote:
>>
>> Having unmovable pages on a given pageblock should be reported correctly
>> when required with REPORT_FAILURE flag. But there can be a scenario where a
>> reserved page in the page block will get reported as a generic "unmovable"
>> reason code. Instead this should be changed to a more appropriate reason
>> code like "Reserved page".
>
> Isn’t this redundant as it dumps the flags in dump_page() anyway?
Even though page flags does contain reserved bit information, the problem
is that we are explicitly printing the reason for this page dump. In this
case it is caused by the fact that it is a reserved page.
page dumped because: <reason>
The proposed change makes it explicit that the dump is caused because a
non movable page with reserved bit set. It also helps in differentiating
between reserved bit condition and the last one "if (found > count)".
>
>>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: Vlastimil Babka <[email protected]>
>> Cc: Oscar Salvador <[email protected]>
>> Cc: Mel Gorman <[email protected]>
>> Cc: Mike Rapoport <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Pavel Tatashin <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> mm/page_alloc.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 15c2050c629b..fbf93ea119d2 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -8206,8 +8206,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>>
>> page = pfn_to_page(check);
>>
>> - if (PageReserved(page))
>> + if (PageReserved(page)) {
>> + reason = "Reserved page";
>> goto unmovable;
>> + }
>>
>> /*
>> * If the zone is movable and we have ruled out all reserved
>> --
>> 2.20.1
>>
>
On 10/03/2019 03:02 PM, Anshuman Khandual wrote:
>
> On 10/03/2019 02:35 PM, Qian Cai wrote:
>>
>>> On Oct 3, 2019, at 4:10 AM, Anshuman Khandual <[email protected]> wrote:
>>>
>>> Having unmovable pages on a given pageblock should be reported correctly
>>> when required with REPORT_FAILURE flag. But there can be a scenario where a
>>> reserved page in the page block will get reported as a generic "unmovable"
>>> reason code. Instead this should be changed to a more appropriate reason
>>> code like "Reserved page".
>> Isn’t this redundant as it dumps the flags in dump_page() anyway?
> Even though page flags does contain reserved bit information, the problem
> is that we are explicitly printing the reason for this page dump. In this
> case it is caused by the fact that it is a reserved page.
>
> page dumped because: <reason>
>
> The proposed change makes it explicit that the dump is caused because a
> non movable page with reserved bit set. It also helps in differentiating
> between reserved bit condition and the last one "if (found > count)"
Instead, will it better to rename the reason codes as
1. "Unmovable (CMA)"
2. "Unmovable (Reserved)"
3. "Unmovable (Private/non-LRU)"
> On Oct 3, 2019, at 5:32 AM, Anshuman Khandual <[email protected]> wrote:
>
> Even though page flags does contain reserved bit information, the problem
> is that we are explicitly printing the reason for this page dump. In this
> case it is caused by the fact that it is a reserved page.
>
> page dumped because: <reason>
>
> The proposed change makes it explicit that the dump is caused because a
> non movable page with reserved bit set. It also helps in differentiating
> between reserved bit condition and the last one "if (found > count)".
Then, someone later would like to add a reason for every different page flags just because they *think* they are special.
On 10/03/2019 04:49 PM, Qian Cai wrote:
>
>
>> On Oct 3, 2019, at 5:32 AM, Anshuman Khandual <[email protected]> wrote:
>>
>> Even though page flags does contain reserved bit information, the problem
>> is that we are explicitly printing the reason for this page dump. In this
>> case it is caused by the fact that it is a reserved page.
>>
>> page dumped because: <reason>
>>
>> The proposed change makes it explicit that the dump is caused because a
>> non movable page with reserved bit set. It also helps in differentiating
>> between reserved bit condition and the last one "if (found > count)".
>
> Then, someone later would like to add a reason for every different page flags just because they *think* they are special.
>
Ohh, never meant that the 'Reserved' bit is anything special here but it
is a reason to make a page unmovable unlike many other flags.
> On Oct 3, 2019, at 7:31 AM, Anshuman Khandual <[email protected]> wrote:
>
> Ohh, never meant that the 'Reserved' bit is anything special here but it
> is a reason to make a page unmovable unlike many other flags.
But dump_page() is used everywhere, and it is better to reserve “reason” to indicate something more important rather than duplicating the page flags.
Especially, it is trivial enough right now for developers look in the page flags dumping from has_unmovable_pages(), and figure out the exact branching in the code.
On 10/03/2019 05:20 PM, Qian Cai wrote:
>
>
>> On Oct 3, 2019, at 7:31 AM, Anshuman Khandual <[email protected]> wrote:
>>
>> Ohh, never meant that the 'Reserved' bit is anything special here but it
>> is a reason to make a page unmovable unlike many other flags.
>
> But dump_page() is used everywhere, and it is better to reserve “reason” to indicate something more important rather than duplicating the page flags.
>
> Especially, it is trivial enough right now for developers look in the page flags dumping from has_unmovable_pages(), and figure out the exact branching in the code.
>
Will something like this be better ? hugepage_migration_supported() has got
uncertainty depending on platform and huge page size.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 15c2050c629b..8dbc86696515 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8175,7 +8175,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
unsigned long found;
unsigned long iter = 0;
unsigned long pfn = page_to_pfn(page);
- const char *reason = "unmovable page";
+ const char *reason;
/*
* TODO we could make this much more efficient by not checking every
@@ -8194,7 +8194,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
if (is_migrate_cma(migratetype))
return false;
- reason = "CMA page";
+ reason = "Unmovable CMA page";
goto unmovable;
}
@@ -8206,8 +8206,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
page = pfn_to_page(check);
- if (PageReserved(page))
+ if (PageReserved(page)) {
+ reason = "Unmovable reserved page";
goto unmovable;
+ }
/*
* If the zone is movable and we have ruled out all reserved
@@ -8226,8 +8228,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
struct page *head = compound_head(page);
unsigned int skip_pages;
- if (!hugepage_migration_supported(page_hstate(head)))
+ if (!hugepage_migration_supported(page_hstate(head))) {
+ reason = "Unmovable HugeTLB page";
goto unmovable;
+ }
skip_pages = compound_nr(head) - (page - head);
iter += skip_pages - 1;
@@ -8271,8 +8275,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
* is set to both of a memory hole page and a _used_ kernel
* page at boot.
*/
- if (found > count)
+ if (found > count) {
+ reason = "Unmovable non-LRU page";
goto unmovable;
+ }
}
return false;
unmovable:
> On Oct 3, 2019, at 8:01 AM, Anshuman Khandual <[email protected]> wrote:
>
> Will something like this be better ?
Not really. dump_page() will dump PageCompound information anyway, so it is trivial to figure out if went in that path.
> hugepage_migration_supported() has got
> uncertainty depending on platform and huge page size.
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 15c2050c629b..8dbc86696515 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8175,7 +8175,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> unsigned long found;
> unsigned long iter = 0;
> unsigned long pfn = page_to_pfn(page);
> - const char *reason = "unmovable page";
> + const char *reason;
>
> /*
> * TODO we could make this much more efficient by not checking every
> @@ -8194,7 +8194,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> if (is_migrate_cma(migratetype))
> return false;
>
> - reason = "CMA page";
> + reason = "Unmovable CMA page";
> goto unmovable;
> }
>
> @@ -8206,8 +8206,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>
> page = pfn_to_page(check);
>
> - if (PageReserved(page))
> + if (PageReserved(page)) {
> + reason = "Unmovable reserved page";
> goto unmovable;
> + }
>
> /*
> * If the zone is movable and we have ruled out all reserved
> @@ -8226,8 +8228,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> struct page *head = compound_head(page);
> unsigned int skip_pages;
>
> - if (!hugepage_migration_supported(page_hstate(head)))
> + if (!hugepage_migration_supported(page_hstate(head))) {
> + reason = "Unmovable HugeTLB page";
> goto unmovable;
> + }
>
> skip_pages = compound_nr(head) - (page - head);
> iter += skip_pages - 1;
> @@ -8271,8 +8275,10 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> * is set to both of a memory hole page and a _used_ kernel
> * page at boot.
> */
> - if (found > count)
> + if (found > count) {
> + reason = "Unmovable non-LRU page";
> goto unmovable;
> + }
> }
> return false;
> unmovable:
On 03.10.19 14:14, Qian Cai wrote:
>
>
>> On Oct 3, 2019, at 8:01 AM, Anshuman Khandual <[email protected]> wrote:
>>
>> Will something like this be better ?
>
> Not really. dump_page() will dump PageCompound information anyway, so it is trivial to figure out if went in that path.
>
I agree, I use the dump_page() output frequently to identify PG_reserved
pages. No need to duplicate that.
--
Thanks,
David / dhildenb
On Thu 03-10-19 13:40:57, Anshuman Khandual wrote:
> Having unmovable pages on a given pageblock should be reported correctly
> when required with REPORT_FAILURE flag. But there can be a scenario where a
> reserved page in the page block will get reported as a generic "unmovable"
> reason code. Instead this should be changed to a more appropriate reason
> code like "Reserved page".
Others have already pointed out this is just redundant but I will have a
more generic comment on the changelog. There is essentially no
information why the current state is bad/unhelpful and why the chnage is
needed. All you claim is that something is a certain way and then assert
that it should be done differently. That is not how changelogs should
look like.
--
Michal Hocko
SUSE Labs
On 10/04/2019 04:28 PM, Michal Hocko wrote:
> On Thu 03-10-19 13:40:57, Anshuman Khandual wrote:
>> Having unmovable pages on a given pageblock should be reported correctly
>> when required with REPORT_FAILURE flag. But there can be a scenario where a
>> reserved page in the page block will get reported as a generic "unmovable"
>> reason code. Instead this should be changed to a more appropriate reason
>> code like "Reserved page".
>
> Others have already pointed out this is just redundant but I will have a
Sure.
> more generic comment on the changelog. There is essentially no
> information why the current state is bad/unhelpful and why the chnage is
The current state is not necessarily bad or unhelpful. I just though that it
could be improved upon. Some how calling out explicitly only the CMA page
failure case just felt adhoc, where as there are other reasons like HugeTLB
immovability which might depend on other factors apart from just page flags
(though I did not propose that originally).
> needed. All you claim is that something is a certain way and then assert
> that it should be done differently. That is not how changelogs should
> look like.
>
Okay, probably I should have explained more on why "unmovable" is less than
adequate to capture the exact reason for specific failure cases and how
"Reserved Page" instead would been better. But got the point, will improve.
On Fri, 2019-10-04 at 17:14 +0530, Anshuman Khandual wrote:
>
> On 10/04/2019 04:28 PM, Michal Hocko wrote:
> > On Thu 03-10-19 13:40:57, Anshuman Khandual wrote:
> > > Having unmovable pages on a given pageblock should be reported correctly
> > > when required with REPORT_FAILURE flag. But there can be a scenario where a
> > > reserved page in the page block will get reported as a generic "unmovable"
> > > reason code. Instead this should be changed to a more appropriate reason
> > > code like "Reserved page".
> >
> > Others have already pointed out this is just redundant but I will have a
>
> Sure.
>
> > more generic comment on the changelog. There is essentially no
> > information why the current state is bad/unhelpful and why the chnage is
>
> The current state is not necessarily bad or unhelpful. I just though that it
> could be improved upon. Some how calling out explicitly only the CMA page
> failure case just felt adhoc, where as there are other reasons like HugeTLB
> immovability which might depend on other factors apart from just page flags
> (though I did not propose that originally).
>
> > needed. All you claim is that something is a certain way and then assert
> > that it should be done differently. That is not how changelogs should
> > look like.
> >
>
> Okay, probably I should have explained more on why "unmovable" is less than
> adequate to capture the exact reason for specific failure cases and how
> "Reserved Page" instead would been better. But got the point, will improve.
>
It might be a good time to rethink if it is really a good idea to dump_page()
at all inside has_unmovable_pages(). As it is right now, it is a a potential
deadlock between console vs memory offline. More details are in this thread,
https://lore.kernel.org/lkml/[email protected]/
01: [ 672.875392] WARNING: possible circular locking dependency detected
01: [ 672.875394] 5.4.0-rc1-next-20191004+ #64 Not tainted
01: [ 672.875396] ------------------------------------------------------
01: [ 672.875398] test.sh/1724 is trying to acquire lock:
01: [ 672.875400] 0000000052059ec0 (console_owner){-...}, at: console_unlock+0x
01: 328/0xa30
01: [ 672.875406]
01: [ 672.875408] but task is already holding lock:
01: [ 672.875409] 000000006ffd89c8 (&(&zone->lock)->rlock){-.-.}, at: start_iso
01: late_page_range+0x216/0x538
01: [ 672.875415]
01: [ 672.875417] which lock already depends on the new lock.
01: [ 672.875418]
01: [ 672.875419]
01: [ 672.875421] the existing dependency chain (in reverse order) is:
01: [ 672.875423]
01: [ 672.875424] -> #2 (&(&zone->lock)->rlock){-.-.}:
01: [ 672.875430] lock_acquire+0x21a/0x468
01: [ 672.875431] _raw_spin_lock+0x54/0x68
01: [ 672.875433] get_page_from_freelist+0x8b6/0x2d28
01: [ 672.875435] __alloc_pages_nodemask+0x246/0x658
01: [ 672.875437] __get_free_pages+0x34/0x78
01: [ 672.875438] sclp_init+0x106/0x690
01: [ 672.875440] sclp_register+0x2e/0x248
01: [ 672.875442] sclp_rw_init+0x4a/0x70
01: [ 672.875443] sclp_console_init+0x4a/0x1b8
01: [ 672.875445] console_init+0x2c8/0x410
01: [ 672.875447] start_kernel+0x530/0x6a0
01: [ 672.875448] startup_continue+0x70/0xd0
01: [ 672.875449]
01: [ 672.875450] -> #1 (sclp_lock){-.-.}:
01: [ 672.875458] lock_acquire+0x21a/0x468
01: [ 672.875460] _raw_spin_lock_irqsave+0xcc/0xe8
01: [ 672.875462] sclp_add_request+0x34/0x308
01: [ 672.875464] sclp_conbuf_emit+0x100/0x138
01: [ 672.875465] sclp_console_write+0x96/0x3b8
01: [ 672.875467] console_unlock+0x6dc/0xa30
01: [ 672.875469] vprintk_emit+0x184/0x3c8
01: [ 672.875470] vprintk_default+0x44/0x50
01: [ 672.875472] printk+0xa8/0xc0
01: [ 672.875473] iommu_debugfs_setup+0xf2/0x108
01: [ 672.875475] iommu_init+0x6c/0x78
01: [ 672.875477] do_one_initcall+0x162/0x680
01: [ 672.875478] kernel_init_freeable+0x4e8/0x5a8
01: [ 672.875480] kernel_init+0x2a/0x188
01: [ 672.875484] ret_from_fork+0x30/0x34
01: [ 672.875486] kernel_thread_starter+0x0/0xc
01: [ 672.875487]
01: [ 672.875488] -> #0 (console_owner){-...}:
01: [ 672.875495] check_noncircular+0x338/0x3e0
01: [ 672.875496] __lock_acquire+0x1e66/0x2d88
01: [ 672.875498] lock_acquire+0x21a/0x468
01: [ 672.875499] console_unlock+0x3a6/0xa30
01: [ 672.875501] vprintk_emit+0x184/0x3c8
01: [ 672.875503] vprintk_default+0x44/0x50
01: [ 672.875504] printk+0xa8/0xc0
01: [ 672.875506] __dump_page+0x1dc/0x710
01: [ 672.875507] dump_page+0x2e/0x58
01: [ 672.875509] has_unmovable_pages+0x2e8/0x470
01: [ 672.875511] start_isolate_page_range+0x404/0x538
01: [ 672.875513] __offline_pages+0x22c/0x1338
01: [ 672.875514] memory_subsys_offline+0xa6/0xe8
01: [ 672.875516] device_offline+0xe6/0x118
01: [ 672.875517] state_store+0xf0/0x110
01: [ 672.875519] kernfs_fop_write+0x1bc/0x270
01: [ 672.875521] vfs_write+0xce/0x220
01: [ 672.875522] ksys_write+0xea/0x190
01: [ 672.875524] system_call+0xd8/0x2b4
01: [ 672.875525]
01: [ 672.875527] other info that might help us debug this:
01: [ 672.875528]
01: [ 672.875529] Chain exists of:
01: [ 672.875530] console_owner --> sclp_lock --> &(&zone->lock)->rlock
01: [ 672.875538]
01: [ 672.875540] Possible unsafe locking scenario:
01: [ 672.875541]
01: [ 672.875543] CPU0 CPU1
01: [ 672.875544] ---- ----
01: [ 672.875545] lock(&(&zone->lock)->rlock);
01: [ 672.875549] lock(sclp_lock);
01: [ 672.875553] lock(&(&zone->lock)->rlock);
01: [ 672.875557] lock(console_owner);
01: [ 672.875560]
01: [ 672.875562] *** DEADLOCK ***
01: [ 672.875563]
01: [ 672.875564] 9 locks held by test.sh/1724:
01: [ 672.875565] #0: 000000000e925408 (sb_writers#4){.+.+}, at: vfs_write+0x2
01: 06/0x220
01: [ 672.875574] #1: 0000000050aa4280 (&of->mutex){+.+.}, at: kernfs_fop_writ
01: e+0x154/0x270
01: [ 672.875581] #2: 0000000062e5c628 (kn->count#198){.+.+}, at: kernfs_fop_w
01: rite+0x16a/0x270
01: [ 672.875590] #3: 00000000523236a0 (device_hotplug_lock){+.+.}, at: lock_d
01: evice_hotplug_sysfs+0x30/0x80
01: [ 672.875598] #4: 0000000062e70990 (&dev->mutex){....}, at: device_offline
01: +0x78/0x118
01: [ 672.875605] #5: 0000000051fd36b0 (cpu_hotplug_lock.rw_sem){++++}, at: __
01: offline_pages+0xec/0x1338
01: [ 672.875613] #6: 00000000521ca470 (mem_hotplug_lock.rw_sem){++++}, at: pe
01: rcpu_down_write+0x38/0x210
01: [ 672.875620] #7: 000000006ffd89c8 (&(&zone->lock)->rlock){-.-.}, at: star
01: t_isolate_page_range+0x216/0x538
01: [ 672.875628] #8: 000000005205a100 (console_lock){+.+.}, at: vprintk_emit+
01: 0x178/0x3c8
01: [ 672.875635]
01: [ 672.875636] stack backtrace:
01: [ 672.875639] CPU: 1 PID: 1724 Comm: test.sh Not tainted 5.4.0-rc1-next-201
01: 91004+ #64
01: [ 672.875640] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
01: [ 672.875642] Call Trace:
01: [ 672.875644] ([<00000000512ae218>] show_stack+0x110/0x1b0)
01: [ 672.875645] [<0000000051b6d506>] dump_stack+0x126/0x178
01: [ 672.875648] [<00000000513a4b08>] check_noncircular+0x338/0x3e0
01: [ 672.875650] [<00000000513aaaf6>] __lock_acquire+0x1e66/0x2d88
01: [ 672.875652] [<00000000513a7e12>] lock_acquire+0x21a/0x468
01: [ 672.875654] [<00000000513bb2fe>] console_unlock+0x3a6/0xa30
01: [ 672.875655] [<00000000513bde2c>] vprintk_emit+0x184/0x3c8
01: [ 672.875657] [<00000000513be0b4>] vprintk_default+0x44/0x50
01: [ 672.875659] [<00000000513beb60>] printk+0xa8/0xc0
01: [ 672.875661] [<000000005158c364>] __dump_page+0x1dc/0x710
01: [ 672.875663] [<000000005158c8c6>] dump_page+0x2e/0x58
01: [ 672.875665] [<00000000515d87c8>] has_unmovable_pages+0x2e8/0x470
01: [ 672.875667] [<000000005167072c>] start_isolate_page_range+0x404/0x538
01: [ 672.875669] [<0000000051b96de4>] __offline_pages+0x22c/0x1338
01: [ 672.875671] [<0000000051908586>] memory_subsys_offline+0xa6/0xe8
01: [ 672.875673] [<00000000518e561e>] device_offline+0xe6/0x118
01: [ 672.875675] [<0000000051908170>] state_store+0xf0/0x110
01: [ 672.875677] [<0000000051796384>] kernfs_fop_write+0x1bc/0x270
01: [ 672.875679] [<000000005168972e>] vfs_write+0xce/0x220
01: [ 672.875681] [<0000000051689b9a>] ksys_write+0xea/0x190
01: [ 672.875685] [<0000000051ba9990>] system_call+0xd8/0x2b4
01: [ 672.875687] INFO: lockdep is turned off.
On Fri 04-10-19 08:56:16, Qian Cai wrote:
[...]
> It might be a good time to rethink if it is really a good idea to dump_page()
> at all inside has_unmovable_pages(). As it is right now, it is a a potential
> deadlock between console vs memory offline. More details are in this thread,
>
> https://lore.kernel.org/lkml/[email protected]/
Huh. That would imply we cannot do any printk from that path, no?
--
Michal Hocko
SUSE Labs
On Fri, 2019-10-04 at 15:07 +0200, Michal Hocko wrote:
> On Fri 04-10-19 08:56:16, Qian Cai wrote:
> [...]
> > It might be a good time to rethink if it is really a good idea to dump_page()
> > at all inside has_unmovable_pages(). As it is right now, it is a a potential
> > deadlock between console vs memory offline. More details are in this thread,
> >
> > https://lore.kernel.org/lkml/[email protected]/
>
> Huh. That would imply we cannot do any printk from that path, no?
Yes, or use something like printk_deferred() or it needs to rework of the
current console locking which I have no clue yet.
On Fri 04-10-19 09:30:39, Qian Cai wrote:
> On Fri, 2019-10-04 at 15:07 +0200, Michal Hocko wrote:
> > On Fri 04-10-19 08:56:16, Qian Cai wrote:
> > [...]
> > > It might be a good time to rethink if it is really a good idea to dump_page()
> > > at all inside has_unmovable_pages(). As it is right now, it is a a potential
> > > deadlock between console vs memory offline. More details are in this thread,
> > >
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Huh. That would imply we cannot do any printk from that path, no?
>
> Yes, or use something like printk_deferred()
This is just insane. The hotplug code is in no way special wrt printk.
It is never called from the printk code AFAIK and thus there is no real
reason why this particular code should be any special. Not to mention
it calls printk indirectly from a code that is shared with other code
paths.
> or it needs to rework of the current console locking which I have no
> clue yet.
Yes, if the lockdep is really referring to a real deadlock which I
haven't really explored.
--
Michal Hocko
SUSE Labs
On Fri, 2019-10-04 at 15:38 +0200, Michal Hocko wrote:
> On Fri 04-10-19 09:30:39, Qian Cai wrote:
> > On Fri, 2019-10-04 at 15:07 +0200, Michal Hocko wrote:
> > > On Fri 04-10-19 08:56:16, Qian Cai wrote:
> > > [...]
> > > > It might be a good time to rethink if it is really a good idea to dump_page()
> > > > at all inside has_unmovable_pages(). As it is right now, it is a a potential
> > > > deadlock between console vs memory offline. More details are in this thread,
> > > >
> > > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Huh. That would imply we cannot do any printk from that path, no?
> >
> > Yes, or use something like printk_deferred()
>
> This is just insane. The hotplug code is in no way special wrt printk.
> It is never called from the printk code AFAIK and thus there is no real
> reason why this particular code should be any special. Not to mention
> it calls printk indirectly from a code that is shared with other code
> paths.
Basically, printk() while holding the zone_lock will be problematic as console
is doing the opposite as it always needs to allocate some memory. Then, it will
always find some way to form this chain,
console_lock -> * -> zone_lock.
>
> > or it needs to rework of the current console locking which I have no
> > clue yet.
>
> Yes, if the lockdep is really referring to a real deadlock which I
> haven't really explored.
>
On Fri 04-10-19 09:56:00, Qian Cai wrote:
> On Fri, 2019-10-04 at 15:38 +0200, Michal Hocko wrote:
> > On Fri 04-10-19 09:30:39, Qian Cai wrote:
> > > On Fri, 2019-10-04 at 15:07 +0200, Michal Hocko wrote:
> > > > On Fri 04-10-19 08:56:16, Qian Cai wrote:
> > > > [...]
> > > > > It might be a good time to rethink if it is really a good idea to dump_page()
> > > > > at all inside has_unmovable_pages(). As it is right now, it is a a potential
> > > > > deadlock between console vs memory offline. More details are in this thread,
> > > > >
> > > > > https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > Huh. That would imply we cannot do any printk from that path, no?
> > >
> > > Yes, or use something like printk_deferred()
> >
> > This is just insane. The hotplug code is in no way special wrt printk.
> > It is never called from the printk code AFAIK and thus there is no real
> > reason why this particular code should be any special. Not to mention
> > it calls printk indirectly from a code that is shared with other code
> > paths.
>
> Basically, printk() while holding the zone_lock will be problematic as console
> is doing the opposite as it always needs to allocate some memory. Then, it will
> always find some way to form this chain,
>
> console_lock -> * -> zone_lock.
So this is not as much a hotplug specific problem but zone->lock ->
printk -> alloc chain that is a problem, right? Who is doing an
allocation from this atomic context? I do not see any atomic allocation
in kernel/printk/printk.c.
--
Michal Hocko
SUSE Labs
On Fri, 4 Oct 2019 16:41:50 +0200 Michal Hocko <[email protected]> wrote:
> > > This is just insane. The hotplug code is in no way special wrt printk.
> > > It is never called from the printk code AFAIK and thus there is no real
> > > reason why this particular code should be any special. Not to mention
> > > it calls printk indirectly from a code that is shared with other code
> > > paths.
> >
> > Basically, printk() while holding the zone_lock will be problematic as console
> > is doing the opposite as it always needs to allocate some memory. Then, it will
> > always find some way to form this chain,
> >
> > console_lock -> * -> zone_lock.
>
> So this is not as much a hotplug specific problem but zone->lock ->
> printk -> alloc chain that is a problem, right? Who is doing an
> allocation from this atomic context? I do not see any atomic allocation
> in kernel/printk/printk.c.
Apparently some console drivers can do memory allocation on the printk()
path.
This behavior is daft, IMO. Have we identified which ones and looked
into fixing them?
> On Oct 5, 2019, at 5:22 PM, Andrew Morton <[email protected]> wrote:
>
> Apparently some console drivers can do memory allocation on the printk()
> path.
>
> This behavior is daft, IMO. Have we identified which ones and looked
> into fixing them?
Not necessary that simple. It is more of 2+ CPUs required to trigger the deadlock. Please see my v2 patch I sent which has all the information. Especially, the thread link included there which contains a few lockdep splat traces and the s390 one in the patch description.
On 10/04/2019 01:55 PM, David Hildenbrand wrote:
> On 03.10.19 14:14, Qian Cai wrote:
>>
>>
>>> On Oct 3, 2019, at 8:01 AM, Anshuman Khandual <[email protected]> wrote:
>>>
>>> Will something like this be better ?
>>
>> Not really. dump_page() will dump PageCompound information anyway, so it is trivial to figure out if went in that path.
>>
>
> I agree, I use the dump_page() output frequently to identify PG_reserved
> pages. No need to duplicate that.
Here in this path there is a reserved page which is preventing
offlining a memory section but unfortunately dump_page() does
not print page->flags for a reserved page pinned there possibly
through memblock_reserve() during boot.
__offline_pages()
start_isolate_page_range()
set_migratetype_isolate()
has_unmovable_pages()
dump_page()
[ 64.920970] ------------[ cut here ]------------
[ 64.921718] WARNING: CPU: 16 PID: 1116 at mm/page_alloc.c:8298 has_unmovable_pages+0x274/0x2a8
[ 64.923110] Modules linked in:
[ 64.923634] CPU: 16 PID: 1116 Comm: bash Not tainted 5.5.0-rc6-00006-gca544f2a11ae-dirty #281
[ 64.925102] Hardware name: linux,dummy-virt (DT)
[ 64.925905] pstate: 60400085 (nZCv daIf +PAN -UAO)
[ 64.926742] pc : has_unmovable_pages+0x274/0x2a8
[ 64.927554] lr : has_unmovable_pages+0x298/0x2a8
[ 64.928359] sp : ffff800014fd3a00
[ 64.928944] x29: ffff800014fd3a00 x28: fffffe0017640000
[ 64.929875] x27: 0000000000000000 x26: ffff0005fcfcda00
[ 64.930810] x25: 0000000000640000 x24: 0000000000000003
[ 64.931736] x23: 0000000019840000 x22: 0000000000001380
[ 64.932667] x21: ffff800011259000 x20: ffff0005fcfcda00
[ 64.933588] x19: 0000000000661000 x18: 0000000000000010
[ 64.934514] x17: 0000000000000000 x16: 0000000000000000
[ 64.935454] x15: ffffffffffffffff x14: ffff8000118498c8
[ 64.936377] x13: ffff800094fd3797 x12: ffff800014fd379f
[ 64.937304] x11: ffff800011861000 x10: ffff800014fd3720
[ 64.938226] x9 : 00000000ffffffd0 x8 : ffff8000106a60d0
[ 64.939156] x7 : 0000000000000000 x6 : ffff0005fc6261b0
[ 64.940078] x5 : ffff0005fc6261b0 x4 : 0000000000000000
[ 64.941003] x3 : ffff0005fc62cf80 x2 : ffffffffffffec80
[ 64.941927] x1 : ffff800011141b58 x0 : ffff0005fcfcda00
[ 64.942857] Call trace:
[ 64.943298] has_unmovable_pages+0x274/0x2a8
[ 64.944056] start_isolate_page_range+0x258/0x360
[ 64.944879] __offline_pages+0xf4/0x9e8
[ 64.945554] offline_pages+0x10/0x18
[ 64.946189] memory_block_action+0x40/0x1a0
[ 64.946929] memory_subsys_offline+0x4c/0x78
[ 64.947679] device_offline+0x98/0xc8
[ 64.948328] unprobe_store+0xa8/0x158
[ 64.948976] dev_attr_store+0x14/0x28
[ 64.949628] sysfs_kf_write+0x40/0x50
[ 64.950273] kernfs_fop_write+0x108/0x218
[ 64.950983] __vfs_write+0x18/0x40
[ 64.951592] vfs_write+0xb0/0x1d0
[ 64.952175] ksys_write+0x64/0xe8
[ 64.952761] __arm64_sys_write+0x18/0x20
[ 64.953451] el0_svc_common.constprop.2+0x88/0x150
[ 64.954293] el0_svc_handler+0x20/0x80
[ 64.954963] el0_sync_handler+0x118/0x188
[ 64.955669] el0_sync+0x140/0x180
[ 64.956256] ---[ end trace b162b4d1cbea304d ]---
[ 64.957063] page:fffffe0017640000 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0
[ 64.958489] raw: 1ffff80000001000 fffffe0017640008 fffffe0017640008 0000000000000000
[ 64.959839] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
[ 64.961174] page dumped because: unmovable page
The reason is dump_page() does not print page->flags universally
and only does so for KSM, Anon and File pages while excluding
reserved pages at boot. Wondering should not we make printing
page->flags universal ?
- Anshuman
On 14.01.20 09:19, Anshuman Khandual wrote:
>
>
> On 10/04/2019 01:55 PM, David Hildenbrand wrote:
>> On 03.10.19 14:14, Qian Cai wrote:
>>>
>>>
>>>> On Oct 3, 2019, at 8:01 AM, Anshuman Khandual <[email protected]> wrote:
>>>>
>>>> Will something like this be better ?
>>>
>>> Not really. dump_page() will dump PageCompound information anyway, so it is trivial to figure out if went in that path.
>>>
>>
>> I agree, I use the dump_page() output frequently to identify PG_reserved
>> pages. No need to duplicate that.
>
> Here in this path there is a reserved page which is preventing
> offlining a memory section but unfortunately dump_page() does
> not print page->flags for a reserved page pinned there possibly
> through memblock_reserve() during boot.
>
> __offline_pages()
> start_isolate_page_range()
> set_migratetype_isolate()
> has_unmovable_pages()
> dump_page()
>
> [ 64.920970] ------------[ cut here ]------------
> [ 64.921718] WARNING: CPU: 16 PID: 1116 at mm/page_alloc.c:8298 has_unmovable_pages+0x274/0x2a8
> [ 64.923110] Modules linked in:
> [ 64.923634] CPU: 16 PID: 1116 Comm: bash Not tainted 5.5.0-rc6-00006-gca544f2a11ae-dirty #281
> [ 64.925102] Hardware name: linux,dummy-virt (DT)
> [ 64.925905] pstate: 60400085 (nZCv daIf +PAN -UAO)
> [ 64.926742] pc : has_unmovable_pages+0x274/0x2a8
> [ 64.927554] lr : has_unmovable_pages+0x298/0x2a8
> [ 64.928359] sp : ffff800014fd3a00
> [ 64.928944] x29: ffff800014fd3a00 x28: fffffe0017640000
> [ 64.929875] x27: 0000000000000000 x26: ffff0005fcfcda00
> [ 64.930810] x25: 0000000000640000 x24: 0000000000000003
> [ 64.931736] x23: 0000000019840000 x22: 0000000000001380
> [ 64.932667] x21: ffff800011259000 x20: ffff0005fcfcda00
> [ 64.933588] x19: 0000000000661000 x18: 0000000000000010
> [ 64.934514] x17: 0000000000000000 x16: 0000000000000000
> [ 64.935454] x15: ffffffffffffffff x14: ffff8000118498c8
> [ 64.936377] x13: ffff800094fd3797 x12: ffff800014fd379f
> [ 64.937304] x11: ffff800011861000 x10: ffff800014fd3720
> [ 64.938226] x9 : 00000000ffffffd0 x8 : ffff8000106a60d0
> [ 64.939156] x7 : 0000000000000000 x6 : ffff0005fc6261b0
> [ 64.940078] x5 : ffff0005fc6261b0 x4 : 0000000000000000
> [ 64.941003] x3 : ffff0005fc62cf80 x2 : ffffffffffffec80
> [ 64.941927] x1 : ffff800011141b58 x0 : ffff0005fcfcda00
> [ 64.942857] Call trace:
> [ 64.943298] has_unmovable_pages+0x274/0x2a8
> [ 64.944056] start_isolate_page_range+0x258/0x360
> [ 64.944879] __offline_pages+0xf4/0x9e8
> [ 64.945554] offline_pages+0x10/0x18
> [ 64.946189] memory_block_action+0x40/0x1a0
> [ 64.946929] memory_subsys_offline+0x4c/0x78
> [ 64.947679] device_offline+0x98/0xc8
> [ 64.948328] unprobe_store+0xa8/0x158
> [ 64.948976] dev_attr_store+0x14/0x28
> [ 64.949628] sysfs_kf_write+0x40/0x50
> [ 64.950273] kernfs_fop_write+0x108/0x218
> [ 64.950983] __vfs_write+0x18/0x40
> [ 64.951592] vfs_write+0xb0/0x1d0
> [ 64.952175] ksys_write+0x64/0xe8
> [ 64.952761] __arm64_sys_write+0x18/0x20
> [ 64.953451] el0_svc_common.constprop.2+0x88/0x150
> [ 64.954293] el0_svc_handler+0x20/0x80
> [ 64.954963] el0_sync_handler+0x118/0x188
> [ 64.955669] el0_sync+0x140/0x180
> [ 64.956256] ---[ end trace b162b4d1cbea304d ]---
> [ 64.957063] page:fffffe0017640000 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0
> [ 64.958489] raw: 1ffff80000001000 fffffe0017640008 fffffe0017640008 0000000000000000
> [ 64.959839] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
> [ 64.961174] page dumped because: unmovable page
>
> The reason is dump_page() does not print page->flags universally
> and only does so for KSM, Anon and File pages while excluding
> reserved pages at boot. Wondering should not we make printing
> page->flags universal ?
The thing is that "PageReserved" on a random page tells us that the
values in the memmap cannot be trusted (in some scenarios).
However, we also expose flags for reserved pages via stable_page_flags()
- /proc/kpageflags. As this is just a debugging mechanism, I think it
makes sense to also print them.
--
Thanks,
David / dhildenb
[Cc Ralph]
On Tue 14-01-20 13:49:14, Anshuman Khandual wrote:
>
>
> On 10/04/2019 01:55 PM, David Hildenbrand wrote:
> > On 03.10.19 14:14, Qian Cai wrote:
> >>
> >>
> >>> On Oct 3, 2019, at 8:01 AM, Anshuman Khandual <[email protected]> wrote:
> >>>
> >>> Will something like this be better ?
> >>
> >> Not really. dump_page() will dump PageCompound information anyway, so it is trivial to figure out if went in that path.
> >>
> >
> > I agree, I use the dump_page() output frequently to identify PG_reserved
> > pages. No need to duplicate that.
>
> Here in this path there is a reserved page which is preventing
> offlining a memory section but unfortunately dump_page() does
> not print page->flags for a reserved page pinned there possibly
> through memblock_reserve() during boot.
>
> __offline_pages()
> start_isolate_page_range()
> set_migratetype_isolate()
> has_unmovable_pages()
> dump_page()
>
> [ 64.920970] ------------[ cut here ]------------
> [ 64.921718] WARNING: CPU: 16 PID: 1116 at mm/page_alloc.c:8298 has_unmovable_pages+0x274/0x2a8
> [ 64.923110] Modules linked in:
> [ 64.923634] CPU: 16 PID: 1116 Comm: bash Not tainted 5.5.0-rc6-00006-gca544f2a11ae-dirty #281
> [ 64.925102] Hardware name: linux,dummy-virt (DT)
> [ 64.925905] pstate: 60400085 (nZCv daIf +PAN -UAO)
> [ 64.926742] pc : has_unmovable_pages+0x274/0x2a8
> [ 64.927554] lr : has_unmovable_pages+0x298/0x2a8
> [ 64.928359] sp : ffff800014fd3a00
> [ 64.928944] x29: ffff800014fd3a00 x28: fffffe0017640000
> [ 64.929875] x27: 0000000000000000 x26: ffff0005fcfcda00
> [ 64.930810] x25: 0000000000640000 x24: 0000000000000003
> [ 64.931736] x23: 0000000019840000 x22: 0000000000001380
> [ 64.932667] x21: ffff800011259000 x20: ffff0005fcfcda00
> [ 64.933588] x19: 0000000000661000 x18: 0000000000000010
> [ 64.934514] x17: 0000000000000000 x16: 0000000000000000
> [ 64.935454] x15: ffffffffffffffff x14: ffff8000118498c8
> [ 64.936377] x13: ffff800094fd3797 x12: ffff800014fd379f
> [ 64.937304] x11: ffff800011861000 x10: ffff800014fd3720
> [ 64.938226] x9 : 00000000ffffffd0 x8 : ffff8000106a60d0
> [ 64.939156] x7 : 0000000000000000 x6 : ffff0005fc6261b0
> [ 64.940078] x5 : ffff0005fc6261b0 x4 : 0000000000000000
> [ 64.941003] x3 : ffff0005fc62cf80 x2 : ffffffffffffec80
> [ 64.941927] x1 : ffff800011141b58 x0 : ffff0005fcfcda00
> [ 64.942857] Call trace:
> [ 64.943298] has_unmovable_pages+0x274/0x2a8
> [ 64.944056] start_isolate_page_range+0x258/0x360
> [ 64.944879] __offline_pages+0xf4/0x9e8
> [ 64.945554] offline_pages+0x10/0x18
> [ 64.946189] memory_block_action+0x40/0x1a0
> [ 64.946929] memory_subsys_offline+0x4c/0x78
> [ 64.947679] device_offline+0x98/0xc8
> [ 64.948328] unprobe_store+0xa8/0x158
> [ 64.948976] dev_attr_store+0x14/0x28
> [ 64.949628] sysfs_kf_write+0x40/0x50
> [ 64.950273] kernfs_fop_write+0x108/0x218
> [ 64.950983] __vfs_write+0x18/0x40
> [ 64.951592] vfs_write+0xb0/0x1d0
> [ 64.952175] ksys_write+0x64/0xe8
> [ 64.952761] __arm64_sys_write+0x18/0x20
> [ 64.953451] el0_svc_common.constprop.2+0x88/0x150
> [ 64.954293] el0_svc_handler+0x20/0x80
> [ 64.954963] el0_sync_handler+0x118/0x188
> [ 64.955669] el0_sync+0x140/0x180
> [ 64.956256] ---[ end trace b162b4d1cbea304d ]---
> [ 64.957063] page:fffffe0017640000 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0
> [ 64.958489] raw: 1ffff80000001000 fffffe0017640008 fffffe0017640008 0000000000000000
> [ 64.959839] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
> [ 64.961174] page dumped because: unmovable page
>
> The reason is dump_page() does not print page->flags universally
> and only does so for KSM, Anon and File pages while excluding
> reserved pages at boot. Wondering should not we make printing
> page->flags universal ?
We used to do that and this caught me as a surprise when looking again.
This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
extra line") which is a cleanup patch and I suspect this result was not
anticipated.
The following will do the trick but I cannot really say I like the code
duplication. pr_cont in this case sounds like a much cleaner solution to
me.
diff --git a/mm/debug.c b/mm/debug.c
index 0461df1207cb..4cbf30d03c88 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -89,6 +89,8 @@ void __dump_page(struct page *page, const char *reason)
} else
pr_warn("%ps\n", mapping->a_ops);
pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
+ } else {
+ pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
}
BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
--
Michal Hocko
SUSE Labs
On 1/14/20 10:10 AM, Michal Hocko wrote:
> [Cc Ralph]
>> The reason is dump_page() does not print page->flags universally
>> and only does so for KSM, Anon and File pages while excluding
>> reserved pages at boot. Wondering should not we make printing
>> page->flags universal ?
>
> We used to do that and this caught me as a surprise when looking again.
> This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
> extra line") which is a cleanup patch and I suspect this result was not
> anticipated.
>
> The following will do the trick but I cannot really say I like the code
> duplication. pr_cont in this case sounds like a much cleaner solution to
> me.
How about this then?
diff --git mm/debug.c mm/debug.c
index 0461df1207cb..6a52316af839 100644
--- mm/debug.c
+++ mm/debug.c
@@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
struct address_space *mapping;
bool page_poisoned = PagePoisoned(page);
int mapcount;
+ char *type = "";
/*
* If struct page is poisoned don't access Page*() functions as that
@@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
page, page_ref_count(page), mapcount,
page->mapping, page_to_pgoff(page));
if (PageKsm(page))
- pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
+ type = "ksm ";
else if (PageAnon(page))
- pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
+ type = "anon ";
else if (mapping) {
if (mapping->host && mapping->host->i_dentry.first) {
struct dentry *dentry;
@@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
} else
pr_warn("%ps\n", mapping->a_ops);
- pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
}
BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
+ pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
+
hex_only:
print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
sizeof(unsigned long), page,
On 01/14/2020 03:53 PM, Vlastimil Babka wrote:
> On 1/14/20 10:10 AM, Michal Hocko wrote:
>> [Cc Ralph]
>>> The reason is dump_page() does not print page->flags universally
>>> and only does so for KSM, Anon and File pages while excluding
>>> reserved pages at boot. Wondering should not we make printing
>>> page->flags universal ?
>>
>> We used to do that and this caught me as a surprise when looking again.
>> This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
>> extra line") which is a cleanup patch and I suspect this result was not
>> anticipated.
>>
>> The following will do the trick but I cannot really say I like the code
>> duplication. pr_cont in this case sounds like a much cleaner solution to
>> me.
>
> How about this then?
This looks better than what we have right now though my initial thought
was similar to what Michal had suggested earlier.
>
> diff --git mm/debug.c mm/debug.c
> index 0461df1207cb..6a52316af839 100644
> --- mm/debug.c
> +++ mm/debug.c
> @@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
> struct address_space *mapping;
> bool page_poisoned = PagePoisoned(page);
> int mapcount;
> + char *type = "";
>
> /*
> * If struct page is poisoned don't access Page*() functions as that
> @@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
> page, page_ref_count(page), mapcount,
> page->mapping, page_to_pgoff(page));
> if (PageKsm(page))
> - pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "ksm ";
> else if (PageAnon(page))
> - pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "anon ";
> else if (mapping) {
> if (mapping->host && mapping->host->i_dentry.first) {
> struct dentry *dentry;
> @@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
> pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
> } else
> pr_warn("%ps\n", mapping->a_ops);
> - pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
> }
> BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>
> + pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
> +
> hex_only:
> print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
> sizeof(unsigned long), page,
>
>
On Tue 14-01-20 11:23:52, Vlastimil Babka wrote:
> On 1/14/20 10:10 AM, Michal Hocko wrote:
> > [Cc Ralph]
> >> The reason is dump_page() does not print page->flags universally
> >> and only does so for KSM, Anon and File pages while excluding
> >> reserved pages at boot. Wondering should not we make printing
> >> page->flags universal ?
> >
> > We used to do that and this caught me as a surprise when looking again.
> > This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
> > extra line") which is a cleanup patch and I suspect this result was not
> > anticipated.
> >
> > The following will do the trick but I cannot really say I like the code
> > duplication. pr_cont in this case sounds like a much cleaner solution to
> > me.
>
> How about this then?
Yes makes sense as well.
> diff --git mm/debug.c mm/debug.c
> index 0461df1207cb..6a52316af839 100644
> --- mm/debug.c
> +++ mm/debug.c
> @@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
> struct address_space *mapping;
> bool page_poisoned = PagePoisoned(page);
> int mapcount;
> + char *type = "";
>
> /*
> * If struct page is poisoned don't access Page*() functions as that
> @@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
> page, page_ref_count(page), mapcount,
> page->mapping, page_to_pgoff(page));
> if (PageKsm(page))
> - pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "ksm ";
> else if (PageAnon(page))
> - pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "anon ";
> else if (mapping) {
> if (mapping->host && mapping->host->i_dentry.first) {
> struct dentry *dentry;
> @@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
> pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
> } else
> pr_warn("%ps\n", mapping->a_ops);
> - pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
> }
> BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>
> + pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
> +
> hex_only:
> print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
> sizeof(unsigned long), page,
>
--
Michal Hocko
SUSE Labs
On 1/14/20 12:32 PM, Michal Hocko wrote:
> On Tue 14-01-20 11:23:52, Vlastimil Babka wrote:
>> On 1/14/20 10:10 AM, Michal Hocko wrote:
>> > [Cc Ralph]
>> >> The reason is dump_page() does not print page->flags universally
>> >> and only does so for KSM, Anon and File pages while excluding
>> >> reserved pages at boot. Wondering should not we make printing
>> >> page->flags universal ?
>> >
>> > We used to do that and this caught me as a surprise when looking again.
>> > This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
>> > extra line") which is a cleanup patch and I suspect this result was not
>> > anticipated.
>> >
>> > The following will do the trick but I cannot really say I like the code
>> > duplication. pr_cont in this case sounds like a much cleaner solution to
>> > me.
>>
>> How about this then?
>
> Yes makes sense as well.
Ok here's a proper formatted patch
----8<----
From 7b673c45bc16526586ae8ea6fba416a547baa04e Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <[email protected]>
Date: Tue, 14 Jan 2020 12:52:48 +0100
Subject: [PATCH] mm, debug: always print flags in dump_page()
Commit 76a1850e4572 ("mm/debug.c: __dump_page() prints an extra line")
inadvertently removed printing of page flags for pages that are neither
anon nor ksm nor have a mapping. Fix that.
Using pr_cont() again would be a solution, but the commit explicitly removed
its use. Avoiding the danger of mixing up split lines from multiple CPUs might
be beneficial for near-panic dumps like this, so fix this without reintroducing
pr_cont().
Reported-by: Anshuman Khandual <[email protected]>
Reported-by: Michal Hocko <[email protected]>
Fixes: 76a1850e4572 ("mm/debug.c: __dump_page() prints an extra line")
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/debug.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/mm/debug.c b/mm/debug.c
index 0461df1207cb..6a52316af839 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
struct address_space *mapping;
bool page_poisoned = PagePoisoned(page);
int mapcount;
+ char *type = "";
/*
* If struct page is poisoned don't access Page*() functions as that
@@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
page, page_ref_count(page), mapcount,
page->mapping, page_to_pgoff(page));
if (PageKsm(page))
- pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
+ type = "ksm ";
else if (PageAnon(page))
- pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
+ type = "anon ";
else if (mapping) {
if (mapping->host && mapping->host->i_dentry.first) {
struct dentry *dentry;
@@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
} else
pr_warn("%ps\n", mapping->a_ops);
- pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
}
BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
+ pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
+
hex_only:
print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
sizeof(unsigned long), page,
--
2.24.1
On Tue 14-01-20 13:04:31, Vlastimil Babka wrote:
[...]
> >From 7b673c45bc16526586ae8ea6fba416a547baa04e Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <[email protected]>
> Date: Tue, 14 Jan 2020 12:52:48 +0100
> Subject: [PATCH] mm, debug: always print flags in dump_page()
>
> Commit 76a1850e4572 ("mm/debug.c: __dump_page() prints an extra line")
> inadvertently removed printing of page flags for pages that are neither
> anon nor ksm nor have a mapping. Fix that.
>
> Using pr_cont() again would be a solution, but the commit explicitly removed
> its use. Avoiding the danger of mixing up split lines from multiple CPUs might
> be beneficial for near-panic dumps like this, so fix this without reintroducing
> pr_cont().
>
> Reported-by: Anshuman Khandual <[email protected]>
> Reported-by: Michal Hocko <[email protected]>
> Fixes: 76a1850e4572 ("mm/debug.c: __dump_page() prints an extra line")
> Signed-off-by: Vlastimil Babka <[email protected]>
Acked-by: Michal Hocko <[email protected]>
Thanks!
> ---
> mm/debug.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/debug.c b/mm/debug.c
> index 0461df1207cb..6a52316af839 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
> struct address_space *mapping;
> bool page_poisoned = PagePoisoned(page);
> int mapcount;
> + char *type = "";
>
> /*
> * If struct page is poisoned don't access Page*() functions as that
> @@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
> page, page_ref_count(page), mapcount,
> page->mapping, page_to_pgoff(page));
> if (PageKsm(page))
> - pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "ksm ";
> else if (PageAnon(page))
> - pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
> + type = "anon ";
> else if (mapping) {
> if (mapping->host && mapping->host->i_dentry.first) {
> struct dentry *dentry;
> @@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
> pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
> } else
> pr_warn("%ps\n", mapping->a_ops);
> - pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
> }
> BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>
> + pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
> +
> hex_only:
> print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
> sizeof(unsigned long), page,
> --
> 2.24.1
--
Michal Hocko
SUSE Labs
On 1/14/20 3:32 AM, Michal Hocko wrote:
> On Tue 14-01-20 11:23:52, Vlastimil Babka wrote:
>> On 1/14/20 10:10 AM, Michal Hocko wrote:
>>> [Cc Ralph]
>>>> The reason is dump_page() does not print page->flags universally
>>>> and only does so for KSM, Anon and File pages while excluding
>>>> reserved pages at boot. Wondering should not we make printing
>>>> page->flags universal ?
>>>
>>> We used to do that and this caught me as a surprise when looking again.
>>> This is a result of 76a1850e4572 ("mm/debug.c: __dump_page() prints an
>>> extra line") which is a cleanup patch and I suspect this result was not
>>> anticipated.
>>>
>>> The following will do the trick but I cannot really say I like the code
>>> duplication. pr_cont in this case sounds like a much cleaner solution to
>>> me.
>>
>> How about this then?
>
> Yes makes sense as well.
>
>> diff --git mm/debug.c mm/debug.c
>> index 0461df1207cb..6a52316af839 100644
>> --- mm/debug.c
>> +++ mm/debug.c
>> @@ -47,6 +47,7 @@ void __dump_page(struct page *page, const char *reason)
>> struct address_space *mapping;
>> bool page_poisoned = PagePoisoned(page);
>> int mapcount;
>> + char *type = "";
>>
>> /*
>> * If struct page is poisoned don't access Page*() functions as that
>> @@ -78,9 +79,9 @@ void __dump_page(struct page *page, const char *reason)
>> page, page_ref_count(page), mapcount,
>> page->mapping, page_to_pgoff(page));
>> if (PageKsm(page))
>> - pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
>> + type = "ksm ";
>> else if (PageAnon(page))
>> - pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
>> + type = "anon ";
>> else if (mapping) {
>> if (mapping->host && mapping->host->i_dentry.first) {
>> struct dentry *dentry;
>> @@ -88,10 +89,11 @@ void __dump_page(struct page *page, const char *reason)
>> pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
>> } else
>> pr_warn("%ps\n", mapping->a_ops);
>> - pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>> }
>> BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>>
>> + pr_warn("%sflags: %#lx(%pGp)\n", type, page->flags, &page->flags);
>> +
>> hex_only:
>> print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>> sizeof(unsigned long), page,
>>
>
Looks good to me. Thanks for the clean up.
Reviewed-by: Ralph Campbell <[email protected]>