RE: [PATCH v1 4/4] page_reporting: change PAGE_REPORTING_DEFAULT_ORDER to -1

[Michael Kelley]

From: Yuvraj Sakshith <yuvraj.sakshith@xxxxxxxxxxxxxxxx> Sent: Sunday, March 1, 2026 7:33 PM
> 
> On Fri, Feb 27, 2026 at 09:50:15PM +0100, David Hildenbrand (Arm) wrote:
> > On 2/27/26 15:06, Yuvraj Sakshith wrote:
> > > PAGE_REPORTING_DEFAULT_ORDER is now set to zero. This means,
> > > pages of order zero cannot be reported to a client/driver -- as zero
> > > is used to signal a fallback to MAX_PAGE_ORDER.
> > >
> > > Change PAGE_REPORTING_DEFAULT_ORDER to (-1),
> > > so that zero can be used as a valid order with which pages can
> > > be reported.
> > >
> > > Signed-off-by: Yuvraj Sakshith <yuvraj.sakshith@xxxxxxxxxxxxxxxx>
> > > ---
> > >  include/linux/page_reporting.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/page_reporting.h b/include/linux/page_reporting.h
> > > index a7e3e30f2..3eb3e26d8 100644
> > > --- a/include/linux/page_reporting.h
> > > +++ b/include/linux/page_reporting.h
> > > @@ -7,7 +7,7 @@
> > >
> > >  /* This value should always be a power of 2, see page_reporting_cycle() */
> > >  #define PAGE_REPORTING_CAPACITY		32
> > > -#define PAGE_REPORTING_DEFAULT_ORDER	0
> > > +#define PAGE_REPORTING_DEFAULT_ORDER	(-1)
> >
> > No need for the ().
> >
> > Wondering whether we now also want to do in this patch:
> >
> >
> > diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> > index f0042d5743af..d432aadf9d07 100644
> > --- a/mm/page_reporting.c
> > +++ b/mm/page_reporting.c
> > @@ -11,8 +11,7 @@
> >  #include "page_reporting.h"
> >  #include "internal.h"
> >
> > -/* Initialize to an unsupported value */
> > -unsigned int page_reporting_order = -1;
> > +unsigned int page_reporting_order = PAGE_REPORTING_DEFAULT_ORDER;
> >
> >  static int page_order_update_notify(const char *val, const struct
> > kernel_param *kp)
> >  {
> > @@ -369,7 +368,7 @@ int page_reporting_register(struct
> > page_reporting_dev_info *prdev)
> >          * pageblock_order.
> >          */
> >
> > -       if (page_reporting_order == -1) {
> > +       if (page_reporting_order == PAGE_REPORTING_DEFAULT_ORDER) {
> >
> >
> 
> Sure. Now that I think of it, don’t you think the first nested if() will
> always be false? and can be compressed down to just one if()?

I don't think what you propose is correct. The purpose of testing
page_reporting_order for -1 is to see if a page reporting order has
been specified on the kernel boot line. If it has been specified, then
the page reporting order specified in the call to page_reporting_register()
[either a specific value or the default] is ignored and the kernel boot
line value prevails. But if page_reporting_order is -1 here, then
no kernel boot line value was specified, and the value passed to
page_reporting_register() should prevail.

With this in mind, substituting PAGE_REPORTING_DEFAULT_ORDER
for the -1 in the test doesn’t exactly make sense to me. The -1 in the
test doesn't have quite the same meaning as the -1 for
PAGE_REPORTING_DEFAULT_ORDER. You could even use -2 for
the initial value of page_reporting_order, and here in the test, in
order to make that distinction obvious. Or use a separate symbolic
name like PAGE_REPORTING_ORDER_NOT_SET.

Michael Kelley

> 
> -       if (page_reporting_order == PAGE_REPORTING_DEFAULT_ORDER) {
> -               if (prdev->order != PAGE_REPORTING_DEFAULT_ORDER &&
> -                       prdev->order <= MAX_PAGE_ORDER)
> -                       page_reporting_order = prdev->order;
> -               else
> -                       page_reporting_order = pageblock_order;
> -       }
> +       page_reporting_order = pageblock_order;
> +
> +       if (prdev->order != PAGE_REPORTING_DEFAULT_ORDER &&
> +               prdev->order <= MAX_PAGE_ORDER)
> +               page_reporting_order = prdev->order;
> 
> Thanks,
> Yuvraj
> 
> >
> > (and wondering whether we should have called it
> > PAGE_REPORTING_USE_DEFAULT_ORDER to make it clearer that it is not an
> > actual order. Leaving that up to you :) )
> >
> > --
> > Cheers,
> >
> > David