[PATCH] mm: warn only once if page table misaccounting is detected

[PATCH] mm: warn only once if page table misaccounting is detected

[Heiko Carstens]

Use pr_alert_once() instead of pr_alert() if page table misaccounting
has been detected.

If this happens once it is very likely that there will be numerous
other occurrence as well, which would flood dmesg and the console with
hardly any added information. Therefore print the warning only once.

Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/fork.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 07cddff89c7b..c887e9eba89f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -647,8 +647,8 @@ static void check_mm(struct mm_struct *mm)
 	}
 
 	if (mm_pgtables_bytes(mm))
-		pr_alert("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
-				mm_pgtables_bytes(mm));
+		pr_alert_once("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
+			      mm_pgtables_bytes(mm));
 
 #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
 	VM_BUG_ON_MM(mm->pmd_huge_pte, mm);
-- 
2.16.4

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[Kirill A. Shutemov]

On Tue, Nov 27, 2018 at 09:36:03AM +0100, Heiko Carstens wrote:
> Use pr_alert_once() instead of pr_alert() if page table misaccounting
> has been detected.
> 
> If this happens once it is very likely that there will be numerous
> other occurrence as well, which would flood dmesg and the console with
> hardly any added information. Therefore print the warning only once.
> 
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Fair enough.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[Michal Hocko]

On Tue 27-11-18 09:36:03, Heiko Carstens wrote:
> Use pr_alert_once() instead of pr_alert() if page table misaccounting
> has been detected.
> 
> If this happens once it is very likely that there will be numerous
> other occurrence as well, which would flood dmesg and the console with
> hardly any added information. Therefore print the warning only once.

Have you actually experience a flood of these messages? Is one per mm
message really that much? If yes why rss counters do not exhibit the
same problem?

> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/fork.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 07cddff89c7b..c887e9eba89f 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -647,8 +647,8 @@ static void check_mm(struct mm_struct *mm)
>  	}
>  
>  	if (mm_pgtables_bytes(mm))
> -		pr_alert("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
> -				mm_pgtables_bytes(mm));
> +		pr_alert_once("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
> +			      mm_pgtables_bytes(mm));
>  
>  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
>  	VM_BUG_ON_MM(mm->pmd_huge_pte, mm);
> -- 
> 2.16.4

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[Heiko Carstens]

On Tue, Nov 27, 2018 at 02:19:16PM +0100, Michal Hocko wrote:
> On Tue 27-11-18 09:36:03, Heiko Carstens wrote:
> > Use pr_alert_once() instead of pr_alert() if page table misaccounting
> > has been detected.
> > 
> > If this happens once it is very likely that there will be numerous
> > other occurrence as well, which would flood dmesg and the console with
> > hardly any added information. Therefore print the warning only once.
> 
> Have you actually experience a flood of these messages? Is one per mm
> message really that much?

Yes, I did. Since in this case all compat processes caused the message
to appear, I saw thousands of these messages.

> If yes why rss counters do not exhibit the same problem?

No rss counter messages appeared. Or do you suggest that the other
pr_alert() within check_mm() should also be changed?

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[Michal Hocko]

On Tue 27-11-18 15:36:38, Heiko Carstens wrote:
> On Tue, Nov 27, 2018 at 02:19:16PM +0100, Michal Hocko wrote:
> > On Tue 27-11-18 09:36:03, Heiko Carstens wrote:
> > > Use pr_alert_once() instead of pr_alert() if page table misaccounting
> > > has been detected.
> > > 
> > > If this happens once it is very likely that there will be numerous
> > > other occurrence as well, which would flood dmesg and the console with
> > > hardly any added information. Therefore print the warning only once.
> > 
> > Have you actually experience a flood of these messages? Is one per mm
> > message really that much?
> 
> Yes, I did. Since in this case all compat processes caused the message
> to appear, I saw thousands of these messages.

This means something went colossally wrong and seeing an avalanche of
messages might be actually helpful because you can at least see the
pattern. I wonder whether the underlying issue would be obvious from a
single instance.

Maybe we want ratelimit instead?
 
> > If yes why rss counters do not exhibit the same problem?
> 
> No rss counter messages appeared. Or do you suggest that the other
> pr_alert() within check_mm() should also be changed?

Whatever we go with (and I do not have a strong opinion here) we should
be consistent I believe.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[Sean Christopherson]

On Tue, Nov 27, 2018 at 09:36:03AM +0100, Heiko Carstens wrote:
> Use pr_alert_once() instead of pr_alert() if page table misaccounting
> has been detected.
> 
> If this happens once it is very likely that there will be numerous
> other occurrence as well, which would flood dmesg and the console with
> hardly any added information. Therefore print the warning only once.
> 
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/fork.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 07cddff89c7b..c887e9eba89f 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -647,8 +647,8 @@ static void check_mm(struct mm_struct *mm)
>  	}
>  
>  	if (mm_pgtables_bytes(mm))
> -		pr_alert("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
> -				mm_pgtables_bytes(mm));
> +		pr_alert_once("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
> +			      mm_pgtables_bytes(mm));

I found the print-always behavior to be useful when developing a driver
that mucked with PTEs directly via vmf_insert_pfn() and had issues with
racing against exit_mmap().  It was nice to be able to recompile only
the driver and rely on dmesg to let me know when I messed up yet again.

Would pr_alert_ratelimited() suffice?

>  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
>  	VM_BUG_ON_MM(mm->pmd_huge_pte, mm);
> -- 
> 2.16.4
> 

Re: [PATCH] mm: warn only once if page table misaccounting is detected

[William Kucharski]

> On Nov 27, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Tue, Nov 27, 2018 at 09:36:03AM +0100, Heiko Carstens wrote:
>> Use pr_alert_once() instead of pr_alert() if page table misaccounting
>> has been detected.
>> 
>> If this happens once it is very likely that there will be numerous
>> other occurrence as well, which would flood dmesg and the console with
>> hardly any added information. Therefore print the warning only once.
>> 
>> Cc: Kirill A. Shutemov <kirill@shutemov.name>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>> ---
>> kernel/fork.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 07cddff89c7b..c887e9eba89f 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -647,8 +647,8 @@ static void check_mm(struct mm_struct *mm)
>> 	}
>> 
>> 	if (mm_pgtables_bytes(mm))
>> -		pr_alert("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
>> -				mm_pgtables_bytes(mm));
>> +		pr_alert_once("BUG: non-zero pgtables_bytes on freeing mm: %ld\n",
>> +			      mm_pgtables_bytes(mm));
> 
> I found the print-always behavior to be useful when developing a driver
> that mucked with PTEs directly via vmf_insert_pfn() and had issues with
> racing against exit_mmap().  It was nice to be able to recompile only
> the driver and rely on dmesg to let me know when I messed up yet again.
> 
> Would pr_alert_ratelimited() suffice?

Actually, I really like that idea.

There are certainly times when it is useful to see a cascade of messages, within reason;
one there are so many they overflow the dmesg buffer they're of limited usefulness.

Something like a pr_alert() that could rate limit to a preset value, perhaps a default of
fifty or so, could prove quite useful indeed without being an all or once choice.

    William Kucharski