[PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Charan Teja Reddy]

When the pages are failed to get isolate or migrate, the page owner
information along with page info is dumped. If there are continuous
failures in migration(say page is pinned) or isolation, the log buffer
is simply getting flooded with the page owner information. As most of
the times page info is sufficient to know the causes for failures of
migration or isolation, place the page owner information under DEBUG_VM.

Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
---
 mm/memory_hotplug.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 63b2e46..f48f30d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1326,7 +1326,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 
 		} else {
 			pr_warn("failed to isolate pfn %lx\n", pfn);
-			dump_page(page, "isolation failed");
+			__dump_page(page, "isolation failed");
+#if defined(CONFIG_DEBUG_VM)
+			dump_page_owner(page);
+#endif
 		}
 		put_page(page);
 	}
@@ -1357,7 +1360,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			list_for_each_entry(page, &source, lru) {
 				pr_warn("migrating pfn %lx failed ret:%d ",
 				       page_to_pfn(page), ret);
-				dump_page(page, "migration failure");
+				__dump_page(page, "migration failure");
+#if defined(CONFIG_DEBUG_VM)
+				dump_page_owner(page);
+#endif
 			}
 			putback_movable_pages(&source);
 		}
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[David Hildenbrand]

On 23.11.20 15:03, Charan Teja Reddy wrote:
> When the pages are failed to get isolate or migrate, the page owner
> information along with page info is dumped. If there are continuous
> failures in migration(say page is pinned) or isolation, the log buffer
> is simply getting flooded with the page owner information. As most of
> the times page info is sufficient to know the causes for failures of
> migration or isolation, place the page owner information under DEBUG_VM.
> 
> Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
> ---
>  mm/memory_hotplug.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 63b2e46..f48f30d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1326,7 +1326,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  
>  		} else {
>  			pr_warn("failed to isolate pfn %lx\n", pfn);
> -			dump_page(page, "isolation failed");
> +			__dump_page(page, "isolation failed");
> +#if defined(CONFIG_DEBUG_VM)
> +			dump_page_owner(page);
> +#endif
>  		}
>  		put_page(page);
>  	}
> @@ -1357,7 +1360,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			list_for_each_entry(page, &source, lru) {
>  				pr_warn("migrating pfn %lx failed ret:%d ",
>  				       page_to_pfn(page), ret);
> -				dump_page(page, "migration failure");
> +				__dump_page(page, "migration failure");
> +#if defined(CONFIG_DEBUG_VM)
> +				dump_page_owner(page);
> +#endif
>  			}
>  			putback_movable_pages(&source);
>  		}
> 

It might also make sense to provide an explicit opt-in whether to
pr_warn/dump at all. Most user simply don't care, yet dmesg gets flooded.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Michal Hocko]

On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
> When the pages are failed to get isolate or migrate, the page owner
> information along with page info is dumped. If there are continuous
> failures in migration(say page is pinned) or isolation, the log buffer
> is simply getting flooded with the page owner information. As most of
> the times page info is sufficient to know the causes for failures of
> migration or isolation, place the page owner information under DEBUG_VM.

I do not see why this path is any different from others that call
dump_page. Page owner can add a very valuable information to debug
the underlying reasons for failures here. It is an opt-in debugging
feature which needs to be enabled explicitly. So I would argue users
are ready to accept a lot of data in the kernel log.
 
> Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
> ---
>  mm/memory_hotplug.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 63b2e46..f48f30d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1326,7 +1326,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  
>  		} else {
>  			pr_warn("failed to isolate pfn %lx\n", pfn);
> -			dump_page(page, "isolation failed");
> +			__dump_page(page, "isolation failed");
> +#if defined(CONFIG_DEBUG_VM)
> +			dump_page_owner(page);
> +#endif
>  		}
>  		put_page(page);
>  	}
> @@ -1357,7 +1360,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			list_for_each_entry(page, &source, lru) {
>  				pr_warn("migrating pfn %lx failed ret:%d ",
>  				       page_to_pfn(page), ret);
> -				dump_page(page, "migration failure");
> +				__dump_page(page, "migration failure");
> +#if defined(CONFIG_DEBUG_VM)
> +				dump_page_owner(page);
> +#endif
>  			}
>  			putback_movable_pages(&source);
>  		}
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Charan Teja Kalla]

Thanks Michal!
On 11/23/2020 7:43 PM, Michal Hocko wrote:
> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
>> When the pages are failed to get isolate or migrate, the page owner
>> information along with page info is dumped. If there are continuous
>> failures in migration(say page is pinned) or isolation, the log buffer
>> is simply getting flooded with the page owner information. As most of
>> the times page info is sufficient to know the causes for failures of
>> migration or isolation, place the page owner information under DEBUG_VM.
> 
> I do not see why this path is any different from others that call
> dump_page. Page owner can add a very valuable information to debug
> the underlying reasons for failures here. It is an opt-in debugging
> feature which needs to be enabled explicitly. So I would argue users
> are ready to accept a lot of data in the kernel log.

Just thinking how frequently failures can happen in those paths. In the
memory hotplug path, we can flood the page owner logs just by making one
page pinned. Say If it is anonymous page, the page owner information
shows is something like below, which is not really telling anything
other than how the pinned page is allocated.

page last allocated via order 0, migratetype Movable, gfp_mask
0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
  prep_new_page+0x7c/0x1a4
  get_page_from_freelist+0x1ac/0x1c4
  __alloc_pages_nodemask+0x12c/0x378
  do_anonymous_page+0xac/0x3b4
  handle_pte_fault+0x2a4/0x3bc
  __handle_speculative_fault+0x208/0x3c0
  do_page_fault+0x280/0x508
  do_translation_fault+0x3c/0x54
  do_mem_abort+0x64/0xf4
  el0_da+0x1c/0x20
 page last free stack trace:
  free_pcp_prepare+0x320/0x454
  free_unref_page_list+0x9c/0x2a4
  release_pages+0x370/0x3c8
  free_pages_and_swap_cache+0xdc/0x10c
  tlb_flush_mmu+0x110/0x134
  tlb_finish_mmu+0x48/0xc0
  unmap_region+0x104/0x138
  __do_munmap+0x2ec/0x3b4
  __arm64_sys_munmap+0x80/0xd8

I see at some places in the kernel where they put the dump_page under
DEBUG_VM, but in the end I agree that it is up to the users need. Then
there are some users who don't care for these page owner logs.

And an issue on Embedded systems with these continuous logs being
printed to the console is the watchdog timeouts, because console logging
happens by disabling the interrupts.

>  
>> Signed-off-by: Charan Teja Reddy <charante@codeaurora.org>
>> ---
>>  mm/memory_hotplug.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 63b2e46..f48f30d 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1326,7 +1326,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  
>>  		} else {
>>  			pr_warn("failed to isolate pfn %lx\n", pfn);
>> -			dump_page(page, "isolation failed");
>> +			__dump_page(page, "isolation failed");
>> +#if defined(CONFIG_DEBUG_VM)
>> +			dump_page_owner(page);
>> +#endif
>>  		}
>>  		put_page(page);
>>  	}
>> @@ -1357,7 +1360,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  			list_for_each_entry(page, &source, lru) {
>>  				pr_warn("migrating pfn %lx failed ret:%d ",
>>  				       page_to_pfn(page), ret);
>> -				dump_page(page, "migration failure");
>> +				__dump_page(page, "migration failure");
>> +#if defined(CONFIG_DEBUG_VM)
>> +				dump_page_owner(page);
>> +#endif
>>  			}
>>  			putback_movable_pages(&source);
>>  		}
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of the Code Aurora Forum, hosted by The Linux Foundation
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Michal Hocko]

On Mon 23-11-20 20:40:40, Charan Teja Kalla wrote:
> 
> Thanks Michal!
> On 11/23/2020 7:43 PM, Michal Hocko wrote:
> > On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
> >> When the pages are failed to get isolate or migrate, the page owner
> >> information along with page info is dumped. If there are continuous
> >> failures in migration(say page is pinned) or isolation, the log buffer
> >> is simply getting flooded with the page owner information. As most of
> >> the times page info is sufficient to know the causes for failures of
> >> migration or isolation, place the page owner information under DEBUG_VM.
> > 
> > I do not see why this path is any different from others that call
> > dump_page. Page owner can add a very valuable information to debug
> > the underlying reasons for failures here. It is an opt-in debugging
> > feature which needs to be enabled explicitly. So I would argue users
> > are ready to accept a lot of data in the kernel log.
> 
> Just thinking how frequently failures can happen in those paths. In the
> memory hotplug path, we can flood the page owner logs just by making one
> page pinned.

If you are operating on a movable zone then pages shouldn't be pinned
for unbound amount of time. Yeah there are some ways to break this
fundamental assumption but this is a bigger problem that needs a
solution.

> Say If it is anonymous page, the page owner information
> shows is something like below, which is not really telling anything
> other than how the pinned page is allocated.

Well you can tell an anonymous page from __dump_page, all right, but
this is not true universally.

> page last allocated via order 0, migratetype Movable, gfp_mask
> 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
>   prep_new_page+0x7c/0x1a4
>   get_page_from_freelist+0x1ac/0x1c4
>   __alloc_pages_nodemask+0x12c/0x378
>   do_anonymous_page+0xac/0x3b4
>   handle_pte_fault+0x2a4/0x3bc
>   __handle_speculative_fault+0x208/0x3c0
>   do_page_fault+0x280/0x508
>   do_translation_fault+0x3c/0x54
>   do_mem_abort+0x64/0xf4
>   el0_da+0x1c/0x20
>  page last free stack trace:
>   free_pcp_prepare+0x320/0x454
>   free_unref_page_list+0x9c/0x2a4
>   release_pages+0x370/0x3c8
>   free_pages_and_swap_cache+0xdc/0x10c
>   tlb_flush_mmu+0x110/0x134
>   tlb_finish_mmu+0x48/0xc0
>   unmap_region+0x104/0x138
>   __do_munmap+0x2ec/0x3b4
>   __arm64_sys_munmap+0x80/0xd8
> 
> I see at some places in the kernel where they put the dump_page under
> DEBUG_VM, but in the end I agree that it is up to the users need. Then
> there are some users who don't care for these page owner logs.

Well, as I've said page_owner requires an explicit enabling and I would
expect that if somebody enables this tracking then it is expected to see
the information when we dump a page state.

> And an issue on Embedded systems with these continuous logs being
> printed to the console is the watchdog timeouts, because console logging
> happens by disabling the interrupts.

Are you enabling page_owner on those systems unconditionally?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Charan Teja Kalla]

On 11/24/2020 1:11 PM, Michal Hocko wrote:
> On Mon 23-11-20 20:40:40, Charan Teja Kalla wrote:
>>
>> Thanks Michal!
>> On 11/23/2020 7:43 PM, Michal Hocko wrote:
>>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
>>>> When the pages are failed to get isolate or migrate, the page owner
>>>> information along with page info is dumped. If there are continuous
>>>> failures in migration(say page is pinned) or isolation, the log buffer
>>>> is simply getting flooded with the page owner information. As most of
>>>> the times page info is sufficient to know the causes for failures of
>>>> migration or isolation, place the page owner information under DEBUG_VM.
>>>
>>> I do not see why this path is any different from others that call
>>> dump_page. Page owner can add a very valuable information to debug
>>> the underlying reasons for failures here. It is an opt-in debugging
>>> feature which needs to be enabled explicitly. So I would argue users
>>> are ready to accept a lot of data in the kernel log.
>>
>> Just thinking how frequently failures can happen in those paths. In the
>> memory hotplug path, we can flood the page owner logs just by making one
>> page pinned.
> 
> If you are operating on a movable zone then pages shouldn't be pinned
> for unbound amount of time. Yeah there are some ways to break this
> fundamental assumption but this is a bigger problem that needs a
> solution.
> 
>> Say If it is anonymous page, the page owner information
>> shows is something like below, which is not really telling anything
>> other than how the pinned page is allocated.
> 
> Well you can tell an anonymous page from __dump_page, all right, but
> this is not true universally.
> 
>> page last allocated via order 0, migratetype Movable, gfp_mask
>> 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
>>   prep_new_page+0x7c/0x1a4
>>   get_page_from_freelist+0x1ac/0x1c4
>>   __alloc_pages_nodemask+0x12c/0x378
>>   do_anonymous_page+0xac/0x3b4
>>   handle_pte_fault+0x2a4/0x3bc
>>   __handle_speculative_fault+0x208/0x3c0
>>   do_page_fault+0x280/0x508
>>   do_translation_fault+0x3c/0x54
>>   do_mem_abort+0x64/0xf4
>>   el0_da+0x1c/0x20
>>  page last free stack trace:
>>   free_pcp_prepare+0x320/0x454
>>   free_unref_page_list+0x9c/0x2a4
>>   release_pages+0x370/0x3c8
>>   free_pages_and_swap_cache+0xdc/0x10c
>>   tlb_flush_mmu+0x110/0x134
>>   tlb_finish_mmu+0x48/0xc0
>>   unmap_region+0x104/0x138
>>   __do_munmap+0x2ec/0x3b4
>>   __arm64_sys_munmap+0x80/0xd8
>>
>> I see at some places in the kernel where they put the dump_page under
>> DEBUG_VM, but in the end I agree that it is up to the users need. Then
>> there are some users who don't care for these page owner logs.
> 
> Well, as I've said page_owner requires an explicit enabling and I would
> expect that if somebody enables this tracking then it is expected to see
> the information when we dump a page state.
> 
>> And an issue on Embedded systems with these continuous logs being
>> printed to the console is the watchdog timeouts, because console logging
>> happens by disabling the interrupts.
> 
> Are you enabling page_owner on those systems unconditionally?
> 

Yes, We do always enable the page owner on just the internal debug
builds for memory analysis, But never on the production kernels. And on
these builds excessive logging, at times because of a pinned page,
causing the watchdog timeouts, is the problem.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Michal Hocko]

On Wed 25-11-20 16:18:06, Charan Teja Kalla wrote:
> 
> 
> On 11/24/2020 1:11 PM, Michal Hocko wrote:
> > On Mon 23-11-20 20:40:40, Charan Teja Kalla wrote:
> >>
> >> Thanks Michal!
> >> On 11/23/2020 7:43 PM, Michal Hocko wrote:
> >>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
> >>>> When the pages are failed to get isolate or migrate, the page owner
> >>>> information along with page info is dumped. If there are continuous
> >>>> failures in migration(say page is pinned) or isolation, the log buffer
> >>>> is simply getting flooded with the page owner information. As most of
> >>>> the times page info is sufficient to know the causes for failures of
> >>>> migration or isolation, place the page owner information under DEBUG_VM.
> >>>
> >>> I do not see why this path is any different from others that call
> >>> dump_page. Page owner can add a very valuable information to debug
> >>> the underlying reasons for failures here. It is an opt-in debugging
> >>> feature which needs to be enabled explicitly. So I would argue users
> >>> are ready to accept a lot of data in the kernel log.
> >>
> >> Just thinking how frequently failures can happen in those paths. In the
> >> memory hotplug path, we can flood the page owner logs just by making one
> >> page pinned.
> > 
> > If you are operating on a movable zone then pages shouldn't be pinned
> > for unbound amount of time. Yeah there are some ways to break this
> > fundamental assumption but this is a bigger problem that needs a
> > solution.
> > 
> >> Say If it is anonymous page, the page owner information
> >> shows is something like below, which is not really telling anything
> >> other than how the pinned page is allocated.
> > 
> > Well you can tell an anonymous page from __dump_page, all right, but
> > this is not true universally.
> > 
> >> page last allocated via order 0, migratetype Movable, gfp_mask
> >> 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
> >>   prep_new_page+0x7c/0x1a4
> >>   get_page_from_freelist+0x1ac/0x1c4
> >>   __alloc_pages_nodemask+0x12c/0x378
> >>   do_anonymous_page+0xac/0x3b4
> >>   handle_pte_fault+0x2a4/0x3bc
> >>   __handle_speculative_fault+0x208/0x3c0
> >>   do_page_fault+0x280/0x508
> >>   do_translation_fault+0x3c/0x54
> >>   do_mem_abort+0x64/0xf4
> >>   el0_da+0x1c/0x20
> >>  page last free stack trace:
> >>   free_pcp_prepare+0x320/0x454
> >>   free_unref_page_list+0x9c/0x2a4
> >>   release_pages+0x370/0x3c8
> >>   free_pages_and_swap_cache+0xdc/0x10c
> >>   tlb_flush_mmu+0x110/0x134
> >>   tlb_finish_mmu+0x48/0xc0
> >>   unmap_region+0x104/0x138
> >>   __do_munmap+0x2ec/0x3b4
> >>   __arm64_sys_munmap+0x80/0xd8
> >>
> >> I see at some places in the kernel where they put the dump_page under
> >> DEBUG_VM, but in the end I agree that it is up to the users need. Then
> >> there are some users who don't care for these page owner logs.
> > 
> > Well, as I've said page_owner requires an explicit enabling and I would
> > expect that if somebody enables this tracking then it is expected to see
> > the information when we dump a page state.
> > 
> >> And an issue on Embedded systems with these continuous logs being
> >> printed to the console is the watchdog timeouts, because console logging
> >> happens by disabling the interrupts.
> > 
> > Are you enabling page_owner on those systems unconditionally?
> > 
> 
> Yes, We do always enable the page owner on just the internal debug
> builds for memory analysis, But never on the production kernels. And on
> these builds excessive logging, at times because of a pinned page,
> causing the watchdog timeouts, is the problem.

OK, I see but I still believe that the debugging might be useful
especially when the owner is not really obvious from the page state.
I also agree that if the output is swapping the logs then the situation
is not really great either. Would something like the below work for your
situation?

MAGIC_NUMBER would need to be somehow figured but I would start with 10
or so. 

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b44d4c7ba73b..3da5c434fb77 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1299,6 +1299,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 	LIST_HEAD(source);
 
 	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
+		int dumped_page = MAGIC_NUMBER;
+
 		if (!pfn_valid(pfn))
 			continue;
 		page = pfn_to_page(pfn);
@@ -1344,7 +1346,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 
 		} else {
 			pr_warn("failed to isolate pfn %lx\n", pfn);
-			dump_page(page, "isolation failed");
+			if (dumped_page--) {
+				dump_page(page, "isolation failed");
+				dumped_page = true;
+			}
 		}
 		put_page(page);
 	}
@@ -1372,10 +1377,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		ret = migrate_pages(&source, alloc_migration_target, NULL,
 			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
 		if (ret) {
+			int dumped_page = MAGIC_NUMBER;
+
 			list_for_each_entry(page, &source, lru) {
 				pr_warn("migrating pfn %lx failed ret:%d ",
 				       page_to_pfn(page), ret);
-				dump_page(page, "migration failure");
+				if (dumped_page--) {
+					dump_page(page, "migration failure");
+				}
 			}
 			putback_movable_pages(&source);
 		}
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Charan Teja Kalla]

Thanks Michal!!

On 11/26/2020 2:48 PM, Michal Hocko wrote:
> On Wed 25-11-20 16:18:06, Charan Teja Kalla wrote:
>>
>>
>> On 11/24/2020 1:11 PM, Michal Hocko wrote:
>>> On Mon 23-11-20 20:40:40, Charan Teja Kalla wrote:
>>>>
>>>> Thanks Michal!
>>>> On 11/23/2020 7:43 PM, Michal Hocko wrote:
>>>>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
>>>>>> When the pages are failed to get isolate or migrate, the page owner
>>>>>> information along with page info is dumped. If there are continuous
>>>>>> failures in migration(say page is pinned) or isolation, the log buffer
>>>>>> is simply getting flooded with the page owner information. As most of
>>>>>> the times page info is sufficient to know the causes for failures of
>>>>>> migration or isolation, place the page owner information under DEBUG_VM.
>>>>>
>>>>> I do not see why this path is any different from others that call
>>>>> dump_page. Page owner can add a very valuable information to debug
>>>>> the underlying reasons for failures here. It is an opt-in debugging
>>>>> feature which needs to be enabled explicitly. So I would argue users
>>>>> are ready to accept a lot of data in the kernel log.
>>>>
>>>> Just thinking how frequently failures can happen in those paths. In the
>>>> memory hotplug path, we can flood the page owner logs just by making one
>>>> page pinned.
>>>
>>> If you are operating on a movable zone then pages shouldn't be pinned
>>> for unbound amount of time. Yeah there are some ways to break this
>>> fundamental assumption but this is a bigger problem that needs a
>>> solution.
>>>
>>>> Say If it is anonymous page, the page owner information
>>>> shows is something like below, which is not really telling anything
>>>> other than how the pinned page is allocated.
>>>
>>> Well you can tell an anonymous page from __dump_page, all right, but
>>> this is not true universally.
>>>
>>>> page last allocated via order 0, migratetype Movable, gfp_mask
>>>> 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
>>>>   prep_new_page+0x7c/0x1a4
>>>>   get_page_from_freelist+0x1ac/0x1c4
>>>>   __alloc_pages_nodemask+0x12c/0x378
>>>>   do_anonymous_page+0xac/0x3b4
>>>>   handle_pte_fault+0x2a4/0x3bc
>>>>   __handle_speculative_fault+0x208/0x3c0
>>>>   do_page_fault+0x280/0x508
>>>>   do_translation_fault+0x3c/0x54
>>>>   do_mem_abort+0x64/0xf4
>>>>   el0_da+0x1c/0x20
>>>>  page last free stack trace:
>>>>   free_pcp_prepare+0x320/0x454
>>>>   free_unref_page_list+0x9c/0x2a4
>>>>   release_pages+0x370/0x3c8
>>>>   free_pages_and_swap_cache+0xdc/0x10c
>>>>   tlb_flush_mmu+0x110/0x134
>>>>   tlb_finish_mmu+0x48/0xc0
>>>>   unmap_region+0x104/0x138
>>>>   __do_munmap+0x2ec/0x3b4
>>>>   __arm64_sys_munmap+0x80/0xd8
>>>>
>>>> I see at some places in the kernel where they put the dump_page under
>>>> DEBUG_VM, but in the end I agree that it is up to the users need. Then
>>>> there are some users who don't care for these page owner logs.
>>>
>>> Well, as I've said page_owner requires an explicit enabling and I would
>>> expect that if somebody enables this tracking then it is expected to see
>>> the information when we dump a page state.
>>>
>>>> And an issue on Embedded systems with these continuous logs being
>>>> printed to the console is the watchdog timeouts, because console logging
>>>> happens by disabling the interrupts.
>>>
>>> Are you enabling page_owner on those systems unconditionally?
>>>
>>
>> Yes, We do always enable the page owner on just the internal debug
>> builds for memory analysis, But never on the production kernels. And on
>> these builds excessive logging, at times because of a pinned page,
>> causing the watchdog timeouts, is the problem.
> 
> OK, I see but I still believe that the debugging might be useful
> especially when the owner is not really obvious from the page state.
> I also agree that if the output is swapping the logs then the situation
> is not really great either. Would something like the below work for your
> situation?
> 
> MAGIC_NUMBER would need to be somehow figured but I would start with 10
> or so. 
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b44d4c7ba73b..3da5c434fb77 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1299,6 +1299,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  	LIST_HEAD(source);
>  
>  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> +		int dumped_page = MAGIC_NUMBER;
> +
>  		if (!pfn_valid(pfn))
>  			continue;
>  		page = pfn_to_page(pfn);
> @@ -1344,7 +1346,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  
>  		} else {
>  			pr_warn("failed to isolate pfn %lx\n", pfn);
> -			dump_page(page, "isolation failed");
> +			if (dumped_page--) {
> +				dump_page(page, "isolation failed");
> +				dumped_page = true;
> +			}
>  		}
>  		put_page(page);
>  	}
> @@ -1372,10 +1377,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		ret = migrate_pages(&source, alloc_migration_target, NULL,
>  			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>  		if (ret) {
> +			int dumped_page = MAGIC_NUMBER;
> +
>  			list_for_each_entry(page, &source, lru) {
>  				pr_warn("migrating pfn %lx failed ret:%d ",
>  				       page_to_pfn(page), ret);
> -				dump_page(page, "migration failure");
> +				if (dumped_page--) {
> +					dump_page(page, "migration failure");
> +				}
>  			}
>  			putback_movable_pages(&source);
>  		}
> 

These are working. Rate limiting these logs with default rate limit
interval and burst also helping me.

Thanks!!

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Michal Hocko]

On Fri 27-11-20 15:53:14, Charan Teja Kalla wrote:
> Thanks Michal!!
> 
> On 11/26/2020 2:48 PM, Michal Hocko wrote:
> > On Wed 25-11-20 16:18:06, Charan Teja Kalla wrote:
> >>
> >>
> >> On 11/24/2020 1:11 PM, Michal Hocko wrote:
> >>> On Mon 23-11-20 20:40:40, Charan Teja Kalla wrote:
> >>>>
> >>>> Thanks Michal!
> >>>> On 11/23/2020 7:43 PM, Michal Hocko wrote:
> >>>>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
> >>>>>> When the pages are failed to get isolate or migrate, the page owner
> >>>>>> information along with page info is dumped. If there are continuous
> >>>>>> failures in migration(say page is pinned) or isolation, the log buffer
> >>>>>> is simply getting flooded with the page owner information. As most of
> >>>>>> the times page info is sufficient to know the causes for failures of
> >>>>>> migration or isolation, place the page owner information under DEBUG_VM.
> >>>>>
> >>>>> I do not see why this path is any different from others that call
> >>>>> dump_page. Page owner can add a very valuable information to debug
> >>>>> the underlying reasons for failures here. It is an opt-in debugging
> >>>>> feature which needs to be enabled explicitly. So I would argue users
> >>>>> are ready to accept a lot of data in the kernel log.
> >>>>
> >>>> Just thinking how frequently failures can happen in those paths. In the
> >>>> memory hotplug path, we can flood the page owner logs just by making one
> >>>> page pinned.
> >>>
> >>> If you are operating on a movable zone then pages shouldn't be pinned
> >>> for unbound amount of time. Yeah there are some ways to break this
> >>> fundamental assumption but this is a bigger problem that needs a
> >>> solution.
> >>>
> >>>> Say If it is anonymous page, the page owner information
> >>>> shows is something like below, which is not really telling anything
> >>>> other than how the pinned page is allocated.
> >>>
> >>> Well you can tell an anonymous page from __dump_page, all right, but
> >>> this is not true universally.
> >>>
> >>>> page last allocated via order 0, migratetype Movable, gfp_mask
> >>>> 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO)
> >>>>   prep_new_page+0x7c/0x1a4
> >>>>   get_page_from_freelist+0x1ac/0x1c4
> >>>>   __alloc_pages_nodemask+0x12c/0x378
> >>>>   do_anonymous_page+0xac/0x3b4
> >>>>   handle_pte_fault+0x2a4/0x3bc
> >>>>   __handle_speculative_fault+0x208/0x3c0
> >>>>   do_page_fault+0x280/0x508
> >>>>   do_translation_fault+0x3c/0x54
> >>>>   do_mem_abort+0x64/0xf4
> >>>>   el0_da+0x1c/0x20
> >>>>  page last free stack trace:
> >>>>   free_pcp_prepare+0x320/0x454
> >>>>   free_unref_page_list+0x9c/0x2a4
> >>>>   release_pages+0x370/0x3c8
> >>>>   free_pages_and_swap_cache+0xdc/0x10c
> >>>>   tlb_flush_mmu+0x110/0x134
> >>>>   tlb_finish_mmu+0x48/0xc0
> >>>>   unmap_region+0x104/0x138
> >>>>   __do_munmap+0x2ec/0x3b4
> >>>>   __arm64_sys_munmap+0x80/0xd8
> >>>>
> >>>> I see at some places in the kernel where they put the dump_page under
> >>>> DEBUG_VM, but in the end I agree that it is up to the users need. Then
> >>>> there are some users who don't care for these page owner logs.
> >>>
> >>> Well, as I've said page_owner requires an explicit enabling and I would
> >>> expect that if somebody enables this tracking then it is expected to see
> >>> the information when we dump a page state.
> >>>
> >>>> And an issue on Embedded systems with these continuous logs being
> >>>> printed to the console is the watchdog timeouts, because console logging
> >>>> happens by disabling the interrupts.
> >>>
> >>> Are you enabling page_owner on those systems unconditionally?
> >>>
> >>
> >> Yes, We do always enable the page owner on just the internal debug
> >> builds for memory analysis, But never on the production kernels. And on
> >> these builds excessive logging, at times because of a pinned page,
> >> causing the watchdog timeouts, is the problem.
> > 
> > OK, I see but I still believe that the debugging might be useful
> > especially when the owner is not really obvious from the page state.
> > I also agree that if the output is swapping the logs then the situation
> > is not really great either. Would something like the below work for your
> > situation?
> > 
> > MAGIC_NUMBER would need to be somehow figured but I would start with 10
> > or so. 
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index b44d4c7ba73b..3da5c434fb77 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1299,6 +1299,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  	LIST_HEAD(source);
> >  
> >  	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> > +		int dumped_page = MAGIC_NUMBER;
> > +
> >  		if (!pfn_valid(pfn))
> >  			continue;
> >  		page = pfn_to_page(pfn);
> > @@ -1344,7 +1346,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  
> >  		} else {
> >  			pr_warn("failed to isolate pfn %lx\n", pfn);
> > -			dump_page(page, "isolation failed");
> > +			if (dumped_page--) {
> > +				dump_page(page, "isolation failed");
> > +				dumped_page = true;
> > +			}
> >  		}
> >  		put_page(page);
> >  	}
> > @@ -1372,10 +1377,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  		ret = migrate_pages(&source, alloc_migration_target, NULL,
> >  			(unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> >  		if (ret) {
> > +			int dumped_page = MAGIC_NUMBER;
> > +
> >  			list_for_each_entry(page, &source, lru) {
> >  				pr_warn("migrating pfn %lx failed ret:%d ",
> >  				       page_to_pfn(page), ret);
> > -				dump_page(page, "migration failure");
> > +				if (dumped_page--) {
> > +					dump_page(page, "migration failure");
> > +				}
> >  			}
> >  			putback_movable_pages(&source);
> >  		}
> > 
> 
> These are working. Rate limiting these logs with default rate limit
> interval and burst also helping me.

Whatever suits you better. I do not have any preference wrt rate
limiting. Feel free to reuse the above.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Vlastimil Babka]

On 11/23/20 4:10 PM, Charan Teja Kalla wrote:
> 
> Thanks Michal!
> On 11/23/2020 7:43 PM, Michal Hocko wrote:
>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
>>> When the pages are failed to get isolate or migrate, the page owner
>>> information along with page info is dumped. If there are continuous
>>> failures in migration(say page is pinned) or isolation, the log buffer
>>> is simply getting flooded with the page owner information. As most of
>>> the times page info is sufficient to know the causes for failures of
>>> migration or isolation, place the page owner information under DEBUG_VM.
>> 
>> I do not see why this path is any different from others that call
>> dump_page. Page owner can add a very valuable information to debug
>> the underlying reasons for failures here. It is an opt-in debugging
>> feature which needs to be enabled explicitly. So I would argue users
>> are ready to accept a lot of data in the kernel log.
> 
> Just thinking how frequently failures can happen in those paths. In the
> memory hotplug path, we can flood the page owner logs just by making one
> page pinned. Say If it is anonymous page, the page owner information

So you say it's flooded when page_owner info is included, but not 
flooded when only the rest of __dump_page() is printed? (which is also 
not just one or two lines). That has to be very specific rate of failures.
Anyway I don't like the solution with arbitrary config option. To 
prevent flooding we generally have ratelimiting, how about that?

Also agreed with Michal that page_owner is explicitly enabled debugging 
option and if you use it in production, that's rather surprising to me, 
and possibly more rare than DEBUG_VM, which IIRC Fedora kernels use.

Re: [PATCH] mm: memory_hotplug: put migration failure information under DEBUG_VM

[Charan Teja Kalla]

Thanks Vlastimil!

On 11/24/2020 7:09 PM, Vlastimil Babka wrote:
> On 11/23/20 4:10 PM, Charan Teja Kalla wrote:
>>
>> Thanks Michal!
>> On 11/23/2020 7:43 PM, Michal Hocko wrote:
>>> On Mon 23-11-20 19:33:16, Charan Teja Reddy wrote:
>>>> When the pages are failed to get isolate or migrate, the page owner
>>>> information along with page info is dumped. If there are continuous
>>>> failures in migration(say page is pinned) or isolation, the log buffer
>>>> is simply getting flooded with the page owner information. As most of
>>>> the times page info is sufficient to know the causes for failures of
>>>> migration or isolation, place the page owner information under
>>>> DEBUG_VM.
>>>
>>> I do not see why this path is any different from others that call
>>> dump_page. Page owner can add a very valuable information to debug
>>> the underlying reasons for failures here. It is an opt-in debugging
>>> feature which needs to be enabled explicitly. So I would argue users
>>> are ready to accept a lot of data in the kernel log.
>>
>> Just thinking how frequently failures can happen in those paths. In the
>> memory hotplug path, we can flood the page owner logs just by making one
>> page pinned. Say If it is anonymous page, the page owner information
> 
> So you say it's flooded when page_owner info is included, but not
> flooded when only the rest of __dump_page() is printed? (which is also
> not just one or two lines). That has to be very specific rate of failures.
> Anyway I don't like the solution with arbitrary config option. To
> prevent flooding we generally have ratelimiting, how about that?
> 

I can still say the logs are flooded with just the __dump_page() but
they are lot lesser compare to dump_page_owner.  The lines are something
like below:
page:ffffffff0b070b80 refcount:3 mapcount:1 mapping:ffffff80faf118e9
index:0xc0903
anon flags:
0x800000000008042c(uptodate|dirty|active|owner_priv_1|swapbacked)
raw: 800000000008042c ffffffc047483a58 ffffffc047483a58 ffffff80faf118e9
raw: 00000000000c0903 00000000000985eb 0000000300000000 ffffff800b5f3000
page dumped because: migration failure
page->mem_cgroup:ffffff800b5f3000
page_owner tracks the page as allocated

Rate limiting the logs, both from __dump_page and dump_page_owner,
looking nice. If it is okay for both of you and Michal,  I can raise the
V2 here.

> Also agreed with Michal that page_owner is explicitly enabled debugging
> option and if you use it in production, that's rather surprising to me,
> and possibly more rare than DEBUG_VM, which IIRC Fedora kernels use.

We just enable it on the internal debug systems but never on the
production kernels.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project