Re: [PATCH] fork: Improve error message for corrupted page tables
On Mon, 2019-08-05 at 15:28 +0200, Vlastimil Babka wrote:
> On 8/2/19 8:46 AM, Prakhya, Sai Praneeth wrote:
> > > > > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > > > > + "MM_FILEPAGES",
> > > > > > + "MM_ANONPAGES",
> > > > > > + "MM_SWAPENTS",
> > > > > > + "MM_SHMEMPAGES",
> > > > > > +};
> > > > >
> > > > > But please let's not put this in a header file. We're asking the
> > > > > compiler to put a copy of all of this into every compilation unit
> > > > > which includes the header. Presumably the compiler is smart enough
> > > > > not to do that, but it's not good practice.
> > > >
> > > > Thanks for the explanation. Makes sense to me.
> > > >
> > > > Just wanted to check before sending V2, Is it OK if I add this to
> > > > kernel/fork.c? or do you have something else in mind?
> > >
> > > I was thinking somewhere like mm/util.c so the array could be used by
> > > other
> > > code. But it seems there is no such code. Perhaps it's best to just
> > > leave fork.c as
> > > it is now.
> >
> > Ok, so does that mean have the struct in header file itself?
>
> If the struct definition (including the string values) was in mm/util.c,
> there would have to be a declaration in a header. If it's in fork.c with
> the only users, there doesn't need to be separate declaration in a header.
Makes sense.
>
> > Sorry! for too many questions. I wanted to check with you before changing
> > because it's *the* fork.c file (I presume random changes will not be
> > encouraged here)
> >
> > I am not yet clear on what's the right thing to do here :(
> > So, could you please help me in deciding.
>
> fork.c should be fine, IMHO
I was leaning to add struct definition in fork.c as well but just wanted to
check with Andrew before posting V2.
Thanks for the reply though :)
Regards,
Sai
Re: [PATCH] fork: Improve error message for corrupted page tables
On 8/2/19 8:46 AM, Prakhya, Sai Praneeth wrote:
> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> + "MM_FILEPAGES",
> + "MM_ANONPAGES",
> + "MM_SWAPENTS",
> + "MM_SHMEMPAGES",
> +};
But please let's not put this in a header file. We're asking the
compiler to put a copy of all of this into every compilation unit
which includes the header. Presumably the compiler is smart enough
not to do that, but it's not good practice.
>>>
>>> Thanks for the explanation. Makes sense to me.
>>>
>>> Just wanted to check before sending V2, Is it OK if I add this to
>>> kernel/fork.c? or do you have something else in mind?
>>
>> I was thinking somewhere like mm/util.c so the array could be used by other
>> code. But it seems there is no such code. Perhaps it's best to just leave
>> fork.c as
>> it is now.
>
> Ok, so does that mean have the struct in header file itself?
If the struct definition (including the string values) was in mm/util.c,
there would have to be a declaration in a header. If it's in fork.c with
the only users, there doesn't need to be separate declaration in a header.
> Sorry! for too many questions. I wanted to check with you before changing
> because it's *the* fork.c file (I presume random changes will not be
> encouraged here)
>
> I am not yet clear on what's the right thing to do here :(
> So, could you please help me in deciding.
fork.c should be fine, IMHO
> Regards,
> Sai
>
RE: [PATCH] fork: Improve error message for corrupted page tables
> >
> > Cc: Ingo Molnar
> > Cc: Peter Zijlstra
> > Cc: Andrew Morton
> > Suggested-by/Acked-by: Dave Hansen
>
> Though I am not sure, should the above be two separate lines instead ?
Sure! Will split them in V2.
>
> > Signed-off-by: Sai Praneeth Prakhya
> > ---
> > include/linux/mm_types_task.h | 7 +++
> > kernel/fork.c | 4 ++--
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mm_types_task.h
> > b/include/linux/mm_types_task.h index d7016dcb245e..881f4ea3a1b5
> > 100644
> > --- a/include/linux/mm_types_task.h
> > +++ b/include/linux/mm_types_task.h
> > @@ -44,6 +44,13 @@ enum {
> > NR_MM_COUNTERS
> > };
> >
> > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > + "MM_FILEPAGES",
> > + "MM_ANONPAGES",
> > + "MM_SWAPENTS",
> > + "MM_SHMEMPAGES",
> > +};
>
> Should index them to match respective typo macros.
>
> [MM_FILEPAGES] = "MM_FILEPAGES",
> [MM_ANONPAGES] = "MM_ANONPAGES",
> [MM_SWAPENTS] = "MM_SWAPENTS",
> [MM_SHMEMPAGES] = "MM_SHMEMPAGES",
Sure! Will change it.
> > +
> > #if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU) #define
> > SPLIT_RSS_COUNTING
> > /* per-thread cached information, */
> > diff --git a/kernel/fork.c b/kernel/fork.c index
> > 2852d0e76ea3..6aef5842d4e0 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
> > long x = atomic_long_read(&mm->rss_stat.count[i]);
> >
> > if (unlikely(x))
> > - printk(KERN_ALERT "BUG: Bad rss-counter state "
> > - "mm:%p idx:%d val:%ld\n", mm, i, x);
> > + pr_alert("BUG: Bad rss-counter state mm:%p type:%s
> val:%ld\n",
> > +mm, resident_page_types[i], x);
> It changes the print function as well, though very minor change but perhaps
> mention that in the commit message ?
Sure! Will mention it in V2.
I have changed printk() to pr_alert() because the other message in check_mm()
uses pr_alert().
Regards,
Sai
RE: [PATCH] fork: Improve error message for corrupted page tables
> > > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > > + "MM_FILEPAGES",
> > > > + "MM_ANONPAGES",
> > > > + "MM_SWAPENTS",
> > > > + "MM_SHMEMPAGES",
> > > > +};
> > >
> > > But please let's not put this in a header file. We're asking the
> > > compiler to put a copy of all of this into every compilation unit
> > > which includes the header. Presumably the compiler is smart enough
> > > not to do that, but it's not good practice.
> >
> > Thanks for the explanation. Makes sense to me.
> >
> > Just wanted to check before sending V2, Is it OK if I add this to
> > kernel/fork.c? or do you have something else in mind?
>
> I was thinking somewhere like mm/util.c so the array could be used by other
> code. But it seems there is no such code. Perhaps it's best to just leave
> fork.c as
> it is now.
Ok, so does that mean have the struct in header file itself?
Sorry! for too many questions. I wanted to check with you before changing
because it's *the* fork.c file (I presume random changes will not be encouraged
here)
I am not yet clear on what's the right thing to do here :(
So, could you please help me in deciding.
Regards,
Sai
Re: [PATCH] fork: Improve error message for corrupted page tables
On 07/31/2019 03:48 AM, Sai Praneeth Prakhya wrote:
> When a user process exits, the kernel cleans up the mm_struct of the user
> process and during cleanup, check_mm() checks the page tables of the user
> process for corruption (E.g: unexpected page flags set/cleared). For
> corrupted page tables, the error message printed by check_mm() isn't very
> clear as it prints the loop index instead of page table type (E.g: Resident
> file mapping pages vs Resident shared memory pages). Hence, improve the
> error message so that it's more informative.
The loop index in check_mm() also happens to be the index in rss_stat[] which
represents individual memory type stats. But you are right, index value here
in the print does not make any sense.
>
> Without patch:
> --
> [ 204.836425] mm/pgtable-generic.c:29: bad p4d
> 89eb4e92(80025f941467)
> [ 204.836544] BUG: Bad rss-counter state mm:f75895ea idx:0 val:2
> [ 204.836615] BUG: Bad rss-counter state mm:f75895ea idx:1 val:5
> [ 204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480
>
> With patch:
> ---
> [ 69.815453] mm/pgtable-generic.c:29: bad p4d
> 84653642(80025ca37467)
> [ 69.815872] BUG: Bad rss-counter state mm:014a6c03
> type:MM_FILEPAGES val:2
> [ 69.815962] BUG: Bad rss-counter state mm:014a6c03
> type:MM_ANONPAGES val:5
> [ 69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480
Yes, this is definitely better.
>
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Andrew Morton
> Suggested-by/Acked-by: Dave Hansen
Though I am not sure, should the above be two separate lines instead ?
> Signed-off-by: Sai Praneeth Prakhya
> ---
> include/linux/mm_types_task.h | 7 +++
> kernel/fork.c | 4 ++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
> index d7016dcb245e..881f4ea3a1b5 100644
> --- a/include/linux/mm_types_task.h
> +++ b/include/linux/mm_types_task.h
> @@ -44,6 +44,13 @@ enum {
> NR_MM_COUNTERS
> };
>
> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> + "MM_FILEPAGES",
> + "MM_ANONPAGES",
> + "MM_SWAPENTS",
> + "MM_SHMEMPAGES",
> +};
Should index them to match respective typo macros.
[MM_FILEPAGES] = "MM_FILEPAGES",
[MM_ANONPAGES] = "MM_ANONPAGES",
[MM_SWAPENTS] = "MM_SWAPENTS",
[MM_SHMEMPAGES] = "MM_SHMEMPAGES",
> +
> #if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU)
> #define SPLIT_RSS_COUNTING
> /* per-thread cached information, */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2852d0e76ea3..6aef5842d4e0 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
> long x = atomic_long_read(&mm->rss_stat.count[i]);
>
> if (unlikely(x))
> - printk(KERN_ALERT "BUG: Bad rss-counter state "
> - "mm:%p idx:%d val:%ld\n", mm, i, x);
> + pr_alert("BUG: Bad rss-counter state mm:%p type:%s
> val:%ld\n",
> + mm, resident_page_types[i], x);
It changes the print function as well, though very minor change but perhaps
mention that in the commit message ?
Re: [PATCH] fork: Improve error message for corrupted page tables
On Wed, 31 Jul 2019 15:36:49 -0700 Sai Praneeth Prakhya
wrote:
> > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > + "MM_FILEPAGES",
> > > + "MM_ANONPAGES",
> > > + "MM_SWAPENTS",
> > > + "MM_SHMEMPAGES",
> > > +};
> >
> > But please let's not put this in a header file. We're asking the
> > compiler to put a copy of all of this into every compilation unit which
> > includes the header. Presumably the compiler is smart enough not to
> > do that, but it's not good practice.
>
> Thanks for the explanation. Makes sense to me.
>
> Just wanted to check before sending V2,
> Is it OK if I add this to kernel/fork.c? or do you have something else in
> mind?
I was thinking somewhere like mm/util.c so the array could be used by
other code. But it seems there is no such code. Perhaps it's best to
just leave fork.c as it is now.
Re: [PATCH] fork: Improve error message for corrupted page tables
> > With patch:
> > ---
> > [ 69.815453] mm/pgtable-generic.c:29: bad p4d
> > 84653642(80025ca37467)
> > [ 69.815872] BUG: Bad rss-counter state mm:014a6c03
> > type:MM_FILEPAGES val:2
> > [ 69.815962] BUG: Bad rss-counter state mm:014a6c03
> > type:MM_ANONPAGES val:5
> > [ 69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480
>
> Seems useful.
>
> > --- a/include/linux/mm_types_task.h
> > +++ b/include/linux/mm_types_task.h
> > @@ -44,6 +44,13 @@ enum {
> > NR_MM_COUNTERS
> > };
> >
> > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > + "MM_FILEPAGES",
> > + "MM_ANONPAGES",
> > + "MM_SWAPENTS",
> > + "MM_SHMEMPAGES",
> > +};
>
> But please let's not put this in a header file. We're asking the
> compiler to put a copy of all of this into every compilation unit which
> includes the header. Presumably the compiler is smart enough not to
> do that, but it's not good practice.
Thanks for the explanation. Makes sense to me.
Just wanted to check before sending V2,
Is it OK if I add this to kernel/fork.c? or do you have something else in
mind?
Regards,
Sai
Re: [PATCH] fork: Improve error message for corrupted page tables
On Tue, 30 Jul 2019 15:18:20 -0700 Sai Praneeth Prakhya
wrote:
> When a user process exits, the kernel cleans up the mm_struct of the user
> process and during cleanup, check_mm() checks the page tables of the user
> process for corruption (E.g: unexpected page flags set/cleared). For
> corrupted page tables, the error message printed by check_mm() isn't very
> clear as it prints the loop index instead of page table type (E.g: Resident
> file mapping pages vs Resident shared memory pages). Hence, improve the
> error message so that it's more informative.
>
> Without patch:
> --
> [ 204.836425] mm/pgtable-generic.c:29: bad p4d
> 89eb4e92(80025f941467)
> [ 204.836544] BUG: Bad rss-counter state mm:f75895ea idx:0 val:2
> [ 204.836615] BUG: Bad rss-counter state mm:f75895ea idx:1 val:5
> [ 204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480
>
> With patch:
> ---
> [ 69.815453] mm/pgtable-generic.c:29: bad p4d
> 84653642(80025ca37467)
> [ 69.815872] BUG: Bad rss-counter state mm:014a6c03
> type:MM_FILEPAGES val:2
> [ 69.815962] BUG: Bad rss-counter state mm:014a6c03
> type:MM_ANONPAGES val:5
> [ 69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480
Seems useful.
> --- a/include/linux/mm_types_task.h
> +++ b/include/linux/mm_types_task.h
> @@ -44,6 +44,13 @@ enum {
> NR_MM_COUNTERS
> };
>
> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> + "MM_FILEPAGES",
> + "MM_ANONPAGES",
> + "MM_SWAPENTS",
> + "MM_SHMEMPAGES",
> +};
But please let's not put this in a header file. We're asking the
compiler to put a copy of all of this into every compilation unit which
includes the header. Presumably the compiler is smart enough not to
do that, but it's not good practice.
[PATCH] fork: Improve error message for corrupted page tables
When a user process exits, the kernel cleans up the mm_struct of the user
process and during cleanup, check_mm() checks the page tables of the user
process for corruption (E.g: unexpected page flags set/cleared). For
corrupted page tables, the error message printed by check_mm() isn't very
clear as it prints the loop index instead of page table type (E.g: Resident
file mapping pages vs Resident shared memory pages). Hence, improve the
error message so that it's more informative.
Without patch:
--
[ 204.836425] mm/pgtable-generic.c:29: bad p4d
89eb4e92(80025f941467)
[ 204.836544] BUG: Bad rss-counter state mm:f75895ea idx:0 val:2
[ 204.836615] BUG: Bad rss-counter state mm:f75895ea idx:1 val:5
[ 204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480
With patch:
---
[ 69.815453] mm/pgtable-generic.c:29: bad p4d
84653642(80025ca37467)
[ 69.815872] BUG: Bad rss-counter state mm:014a6c03 type:MM_FILEPAGES
val:2
[ 69.815962] BUG: Bad rss-counter state mm:014a6c03 type:MM_ANONPAGES
val:5
[ 69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480
Cc: Ingo Molnar
Cc: Peter Zijlstra
Cc: Andrew Morton
Suggested-by/Acked-by: Dave Hansen
Signed-off-by: Sai Praneeth Prakhya
---
include/linux/mm_types_task.h | 7 +++
kernel/fork.c | 4 ++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
index d7016dcb245e..881f4ea3a1b5 100644
--- a/include/linux/mm_types_task.h
+++ b/include/linux/mm_types_task.h
@@ -44,6 +44,13 @@ enum {
NR_MM_COUNTERS
};
+static const char * const resident_page_types[NR_MM_COUNTERS] = {
+ "MM_FILEPAGES",
+ "MM_ANONPAGES",
+ "MM_SWAPENTS",
+ "MM_SHMEMPAGES",
+};
+
#if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU)
#define SPLIT_RSS_COUNTING
/* per-thread cached information, */
diff --git a/kernel/fork.c b/kernel/fork.c
index 2852d0e76ea3..6aef5842d4e0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
long x = atomic_long_read(&mm->rss_stat.count[i]);
if (unlikely(x))
- printk(KERN_ALERT "BUG: Bad rss-counter state "
- "mm:%p idx:%d val:%ld\n", mm, i, x);
+ pr_alert("BUG: Bad rss-counter state mm:%p type:%s
val:%ld\n",
+mm, resident_page_types[i], x);
}
if (mm_pgtables_bytes(mm))
--
2.19.1
