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 <[email protected]> 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
