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 <[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. > 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
