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

[Yuvraj Sakshith]

On Mon, Mar 02, 2026 at 09:09:13AM +0100, David Hildenbrand (Arm) wrote:
> On 3/2/26 09:00, Yuvraj Sakshith wrote:
> > On Mon, Mar 02, 2026 at 08:42:57AM +0100, David Hildenbrand (Arm) wrote:
> >> On 3/2/26 06:25, Michael Kelley wrote:
> >>> From: Yuvraj Sakshith <yuvraj.sakshith@xxxxxxxxxxxxxxxx> Sent: Sunday, March 1, 2026 7:33 PM
> >>>
> >>> 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.
> >>
> > Option 1:
> > 
> > if (page_reporting_order == PAGE_REPORTING_DEFAULT_ORDER) {
> >         if (page_reporting_order != PAGE_REPORTING_DEFAULT_ORDER
> >                 && prdev->order <= MAX_PAGE_ORDER) {
> >                 page_reporting_order = prdev->order;
> >         } else {
> >                 page_reporting_order = pageblock_order;
> >         }
> > }
> > 
> > Option 2:
> > 
> > if (page_reporting_order == PAGE_REPORTING_ORDER_NOT_SET) {
> >         if (page_reporting_order != PAGE_REPORTING_DEFAULT_ORDER
> >                 && prdev->order <= MAX_PAGE_ORDER) {
> >                 page_reporting_order = prdev->order;
> >         } else {
> >                 page_reporting_order = pageblock_order;
> >         }
> > }
> > 
> > 
> >> I don't really see a difference between "PAGE_REPORTING_DEFAULT_ORDER"
> >> and "PAGE_REPORTING_ORDER_NOT_SET" that would warrant a split and adding
> >> confusion for the page-reporting drivers.
> >>
> >> In both cases, we want "no special requirement, just use the default".
> >> Maybe we can use a better name to express that.
> > 
> > Agreed.
> > 
> > If we were to read this code without context, wouldn't it be confusing as to
> > why PAGE_REPORTING_DEFAULT_ORDER is being checked in the first place?
> 
> I proposed in one of the last mail that
> "PAGE_REPORTING_USE_DEFAULT_ORDER" could be clearer, stating that it's
> not really an order just yet. Maybe just using
> PAGE_REPORTING_ORDER_UNSET might be clearer.
> 
Ok
> > 
> > Option 1 checks if page_reporting_order is equal to PAGE_REPORTING_DEFAULT_ORDER
> > and then immediately checks if its not equal to it. Which is a bit confusing..
> 
> 
> Because it's wrong? :) We're not supposed to check page_reporting_order
> a second time. Assume we
> s/PAGE_REPORTING_ORDER/PAGE_REPORTING_ORDER_UNSET/ and actually check
> prdev->order:
Oops, typo :) I meant prdev->order.
> 
> if (page_reporting_order == PAGE_REPORTING_ORDER_UNSET) {
> 	if (prdev->order != PAGE_REPORTING_ORDER_UNSET &&
> 	    prdev->order <= MAX_PAGE_ORDER) {
> 		page_reporting_order = prdev->order;
> 	} else {
> 		page_reporting_order = pageblock_order;
> 	}
> }
> 
Great. Much more clearer on page_reporting.c 's end. 

Don't you think on the driver's end:

prdev->order = PAGE_REPORTING_USE_DEFAULT; looks clearer? As compared to:
prdev->order = PAGE_REPORTING_ORDER_UNSET; ?

I'm thinking, why would a driver worry about page_reporting_order being set/unset?

But yes, too many flags...  


Thanks,
Yuvraj