[RFC,11/15] mm/page_reporting: report pages at section size instead of MAX_ORDER.

Commit Message

Zi Yan Aug. 5, 2021, 7:02 p.m. UTC

From: Zi Yan <[e-mail]>

page_reporting_order was set to MAX_ORDER, which is always smaller than
a memory section size. An upcoming change will make MAX_ORDER larger
than a memory section size. Set page_reporting_order to
PFN_SECTION_SHIFT to match existing size assumption.

Signed-off-by: Zi Yan <[e-mail]>
Cc: David Hildenbrand <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 mm/page_reporting.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

David Hildenbrand Aug. 9, 2021, 7:25 a.m. UTC | #1

On 05.08.21 21:02, Zi Yan wrote:
> From: Zi Yan <[e-mail]>
> 
> page_reporting_order was set to MAX_ORDER, which is always smaller than
> a memory section size. An upcoming change will make MAX_ORDER larger
> than a memory section size. Set page_reporting_order to
> PFN_SECTION_SHIFT to match existing size assumption.
> 
> Signed-off-by: Zi Yan <[e-mail]>
> Cc: David Hildenbrand <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>   mm/page_reporting.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> index 382958eef8a9..dc4a2d699862 100644
> --- a/mm/page_reporting.c
> +++ b/mm/page_reporting.c
> @@ -11,7 +11,8 @@
>   #include "page_reporting.h"
>   #include "internal.h"
>   
> -unsigned int page_reporting_order = MAX_ORDER;
> +/* Set page_reporting_order at section size */
> +unsigned int page_reporting_order = PFN_SECTION_SHIFT;
>   module_param(page_reporting_order, uint, 0644);
>   MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
>   
> 

If you look closely, this is only a placeholder and will get overwritten 
in page_reporting_register(). I don't recall why we have the module 
parameter at all. Most probably, to adjust the reporting order after we 
already registered a user. Can't we just initialize that to 0 ?

Alexander Duyck Aug. 9, 2021, 2:08 p.m. UTC | #2

On Thu, Aug 5, 2021 at 12:04 PM Zi Yan <[e-mail]> wrote:
>
> From: Zi Yan <[e-mail]>
>
> page_reporting_order was set to MAX_ORDER, which is always smaller than
> a memory section size. An upcoming change will make MAX_ORDER larger
> than a memory section size. Set page_reporting_order to
> PFN_SECTION_SHIFT to match existing size assumption.
>
> Signed-off-by: Zi Yan <[e-mail]>
> Cc: David Hildenbrand <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  mm/page_reporting.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> index 382958eef8a9..dc4a2d699862 100644
> --- a/mm/page_reporting.c
> +++ b/mm/page_reporting.c
> @@ -11,7 +11,8 @@
>  #include "page_reporting.h"
>  #include "internal.h"
>
> -unsigned int page_reporting_order = MAX_ORDER;
> +/* Set page_reporting_order at section size */
> +unsigned int page_reporting_order = PFN_SECTION_SHIFT;
>  module_param(page_reporting_order, uint, 0644);
>  MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");

The MAX_ORDER assumption is correct. The general idea with this being
set to MAX_ORDER is that the processing is from page_reporting_order
to MAX_ORDER. So with it set to MAX_ORDER then page reporting is
disabled.

Alexander Duyck Aug. 9, 2021, 2:12 p.m. UTC | #3

On Mon, Aug 9, 2021 at 12:25 AM David Hildenbrand <[e-mail]> wrote:
>
> On 05.08.21 21:02, Zi Yan wrote:
> > From: Zi Yan <[e-mail]>
> >
> > page_reporting_order was set to MAX_ORDER, which is always smaller than
> > a memory section size. An upcoming change will make MAX_ORDER larger
> > than a memory section size. Set page_reporting_order to
> > PFN_SECTION_SHIFT to match existing size assumption.
> >
> > Signed-off-by: Zi Yan <[e-mail]>
> > Cc: David Hildenbrand <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >   mm/page_reporting.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> > index 382958eef8a9..dc4a2d699862 100644
> > --- a/mm/page_reporting.c
> > +++ b/mm/page_reporting.c
> > @@ -11,7 +11,8 @@
> >   #include "page_reporting.h"
> >   #include "internal.h"
> >
> > -unsigned int page_reporting_order = MAX_ORDER;
> > +/* Set page_reporting_order at section size */
> > +unsigned int page_reporting_order = PFN_SECTION_SHIFT;
> >   module_param(page_reporting_order, uint, 0644);
> >   MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
> >
> >
>
> If you look closely, this is only a placeholder and will get overwritten
> in page_reporting_register(). I don't recall why we have the module
> parameter at all. Most probably, to adjust the reporting order after we
> already registered a user. Can't we just initialize that to 0 ?

Yeah, it is pretty much there for debugging in the event that we are
on an architecture that is misconfigured.

Zi Yan Aug. 9, 2021, 3:08 p.m. UTC | #4

On 9 Aug 2021, at 10:12, Alexander Duyck wrote:

> On Mon, Aug 9, 2021 at 12:25 AM David Hildenbrand <[e-mail]> wrote:
>>
>> On 05.08.21 21:02, Zi Yan wrote:
>>> From: Zi Yan <[e-mail]>
>>>
>>> page_reporting_order was set to MAX_ORDER, which is always smaller than
>>> a memory section size. An upcoming change will make MAX_ORDER larger
>>> than a memory section size. Set page_reporting_order to
>>> PFN_SECTION_SHIFT to match existing size assumption.
>>>
>>> Signed-off-by: Zi Yan <[e-mail]>
>>> Cc: David Hildenbrand <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>   mm/page_reporting.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c
>>> index 382958eef8a9..dc4a2d699862 100644
>>> --- a/mm/page_reporting.c
>>> +++ b/mm/page_reporting.c
>>> @@ -11,7 +11,8 @@
>>>   #include "page_reporting.h"
>>>   #include "internal.h"
>>>
>>> -unsigned int page_reporting_order = MAX_ORDER;
>>> +/* Set page_reporting_order at section size */
>>> +unsigned int page_reporting_order = PFN_SECTION_SHIFT;
>>>   module_param(page_reporting_order, uint, 0644);
>>>   MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
>>>
>>>
>>
>> If you look closely, this is only a placeholder and will get overwritten
>> in page_reporting_register(). I don't recall why we have the module
>> parameter at all. Most probably, to adjust the reporting order after we
>> already registered a user. Can't we just initialize that to 0 ?
>
> Yeah, it is pretty much there for debugging in the event that we are
> on an architecture that is misconfigured.

MAX_ORDER is changed to a boot time variable in Patch 15, thus cannot be used
for page_reporting_order initialization after that.

Thanks for David’s explanation. I will initialize page_reporting_order to 0
and fix the commit message.

—
Best Regards,
Yan, Zi

Alexander Duyck Aug. 9, 2021, 4:51 p.m. UTC | #5

On Mon, Aug 9, 2021 at 8:08 AM Zi Yan <[e-mail]> wrote:
>
> On 9 Aug 2021, at 10:12, Alexander Duyck wrote:
>
> > On Mon, Aug 9, 2021 at 12:25 AM David Hildenbrand <[e-mail]> wrote:
> >>
> >> On 05.08.21 21:02, Zi Yan wrote:
> >>> From: Zi Yan <[e-mail]>
> >>>
> >>> page_reporting_order was set to MAX_ORDER, which is always smaller than
> >>> a memory section size. An upcoming change will make MAX_ORDER larger
> >>> than a memory section size. Set page_reporting_order to
> >>> PFN_SECTION_SHIFT to match existing size assumption.
> >>>
> >>> Signed-off-by: Zi Yan <[e-mail]>
> >>> Cc: David Hildenbrand <[e-mail]>
> >>> Cc: [e-mail]
> >>> Cc: [e-mail]
> >>> ---
> >>>   mm/page_reporting.c | 3 ++-
> >>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> >>> index 382958eef8a9..dc4a2d699862 100644
> >>> --- a/mm/page_reporting.c
> >>> +++ b/mm/page_reporting.c
> >>> @@ -11,7 +11,8 @@
> >>>   #include "page_reporting.h"
> >>>   #include "internal.h"
> >>>
> >>> -unsigned int page_reporting_order = MAX_ORDER;
> >>> +/* Set page_reporting_order at section size */
> >>> +unsigned int page_reporting_order = PFN_SECTION_SHIFT;
> >>>   module_param(page_reporting_order, uint, 0644);
> >>>   MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
> >>>
> >>>
> >>
> >> If you look closely, this is only a placeholder and will get overwritten
> >> in page_reporting_register(). I don't recall why we have the module
> >> parameter at all. Most probably, to adjust the reporting order after we
> >> already registered a user. Can't we just initialize that to 0 ?
> >
> > Yeah, it is pretty much there for debugging in the event that we are
> > on an architecture that is misconfigured.
>
> MAX_ORDER is changed to a boot time variable in Patch 15, thus cannot be used
> for page_reporting_order initialization after that.
>
> Thanks for David’s explanation. I will initialize page_reporting_order to 0
> and fix the commit message.

Rather than 0 it might be better to use (unsigned)-1 as that would
prevent page reporting from being able to run until the value is
overwritten.

Patch

diff --git a/mm/page_reporting.c b/mm/page_reporting.c
index 382958eef8a9..dc4a2d699862 100644
--- a/mm/page_reporting.c
+++ b/mm/page_reporting.c
@@ -11,7 +11,8 @@ 
 #include "page_reporting.h"
 #include "internal.h"
 
-unsigned int page_reporting_order = MAX_ORDER;
+/* Set page_reporting_order at section size */
+unsigned int page_reporting_order = PFN_SECTION_SHIFT;
 module_param(page_reporting_order, uint, 0644);
 MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");