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

["David Hildenbrand (Arm)" <david@kernel.org>]

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@oss.qualcomm.com> 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.

> 
> 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:

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;
	}
}




-- 
Cheers,

David