[PATCH v1 0/4] Allow order zero pages in page reporting

[PATCH v1 0/4] Allow order zero pages in page reporting

[Yuvraj Sakshith]

Today, page reporting sets page_reporting_order in two ways:

(1) page_reporting.page_reporting_order cmdline parameter
(2) Driver can pass order while registering itself.

In both cases, order zero is ignored by free page reporting
because it is used to set page_reporting_order to a default
value, like MAX_PAGE_ORDER.

In some cases we might want page_reporting_order to be zero.

For instance, when virtio-balloon runs inside a guest with
tiny memory (say, 16MB), it might not be able to find a order 1 page
(or in the worst case order MAX_PAGE_ORDER page) after some uptime.
Page reporting should be able to return order zero pages back for
optimal memory relinquishment.

This patch changes the default fallback value from '0' to '-1' in
all possible clients of free page reporting (hv_balloon and
virtio-balloon) together with allowing '0' as a valid order in
page_reporting_register().

Changes in v1:
- Introduce PAGE_REPORTING_DEFAULT_ORDER macro (initially set to 0).
- Make use of new macro in drivers (hv_balloon and virtio-balloon)
	working with page reporting.
- Change PAGE_REPORTING_DEFAULT_ORDER to -1 as zero is a valid
	page order that can be requested.

Yuvraj Sakshith (3):
  mm/page_reporting: Allow zero page_reporting_order
  hv_balloon: Change default page reporting order
  virtio_balloon: Set pr_dev.order to new default

 drivers/hv/hv_balloon.c         |  2 +-
 drivers/virtio/virtio_balloon.c | 14 ++++++++++++++
 mm/page_reporting.c             |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v1 1/4] page_reporting: add PAGE_REPORTING_DEFAULT_ORDER

[Yuvraj Sakshith]

Drivers can pass order of pages to be reported while
registering itself. Today, this is a magic number, 0.

Label this with PAGE_REPORTING_DEFAULT_ORDER and
check for it when the driver is being registered.

Signed-off-by: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com>
---
 include/linux/page_reporting.h | 1 +
 mm/page_reporting.c            | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/page_reporting.h b/include/linux/page_reporting.h
index fe648dfa3..a7e3e30f2 100644
--- a/include/linux/page_reporting.h
+++ b/include/linux/page_reporting.h
@@ -7,6 +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
 
 struct page_reporting_dev_info {
 	/* function that alters pages to make them "reported" */
diff --git a/mm/page_reporting.c b/mm/page_reporting.c
index e4c428e61..9ad4fc3f8 100644
--- a/mm/page_reporting.c
+++ b/mm/page_reporting.c
@@ -370,7 +370,8 @@ int page_reporting_register(struct page_reporting_dev_info *prdev)
 	 */
 
 	if (page_reporting_order == -1) {
-		if (prdev->order > 0 && prdev->order <= MAX_PAGE_ORDER)
+		if (prdev->order != PAGE_REPORTING_DEFAULT_ORDER &&
+			prdev->order <= MAX_PAGE_ORDER)
 			page_reporting_order = prdev->order;
 		else
 			page_reporting_order = pageblock_order;
-- 
2.34.1

[PATCH v1 2/4] virtio_balloon: set default page reporting order

[Yuvraj Sakshith]

virtio_balloon page reporting order is set to MAX_PAGE_ORDER implicitly
as vb->prdev.order is never initialised and is auto-set to zero.

Explicitly mention usage of default page order by making use of
PAGE_REPORTING_DEFAULT ORDER fallback value.

Signed-off-by: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com>
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 74fe59f5a..0616c03b2 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1044,6 +1044,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			goto out_unregister_oom;
 		}
 
+		vb->pr_dev_info.order = PAGE_REPORTING_DEFAULT_ORDER;
+
 		/*
 		 * The default page reporting order is @pageblock_order, which
 		 * corresponds to 512MB in size on ARM64 when 64KB base page
-- 
2.34.1

[PATCH v1 3/4] hv_balloon: set default page reporting order

[Yuvraj Sakshith]

Explicitly mention page reporting order to be set to
default value using PAGE_REPORTING_DEFAULT_ORDER fallback
value.

Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
Signed-off-by: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com>
---
 drivers/hv/hv_balloon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 2b4080e51..3d6bd9936 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1663,7 +1663,7 @@ static void enable_page_reporting(void)
 	 * We let the page_reporting_order parameter decide the order
 	 * in the page_reporting code
 	 */
-	dm_device.pr_dev_info.order = 0;
+	dm_device.pr_dev_info.order = PAGE_REPORTING_DEFAULT_ORDER;
 	ret = page_reporting_register(&dm_device.pr_dev_info);
 	if (ret < 0) {
 		dm_device.pr_dev_info.report = NULL;
-- 
2.34.1

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

[Yuvraj Sakshith]

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@oss.qualcomm.com>
---
 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)
 
 struct page_reporting_dev_info {
 	/* function that alters pages to make them "reported" */
-- 
2.34.1

Re: [PATCH v1 0/4] Allow order zero pages in page reporting

[David Hildenbrand (Arm)]

On 2/27/26 15:06, Yuvraj Sakshith wrote:
> Today, page reporting sets page_reporting_order in two ways:
> 
> (1) page_reporting.page_reporting_order cmdline parameter
> (2) Driver can pass order while registering itself.
> 
> In both cases, order zero is ignored by free page reporting
> because it is used to set page_reporting_order to a default
> value, like MAX_PAGE_ORDER.
> 
> In some cases we might want page_reporting_order to be zero.
> 
> For instance, when virtio-balloon runs inside a guest with
> tiny memory (say, 16MB), it might not be able to find a order 1 page
> (or in the worst case order MAX_PAGE_ORDER page) after some uptime.
> Page reporting should be able to return order zero pages back for
> optimal memory relinquishment.
> 
> This patch changes the default fallback value from '0' to '-1' in
> all possible clients of free page reporting (hv_balloon and
> virtio-balloon) together with allowing '0' as a valid order in
> page_reporting_register().
> 
> Changes in v1:
> - Introduce PAGE_REPORTING_DEFAULT_ORDER macro (initially set to 0).
> - Make use of new macro in drivers (hv_balloon and virtio-balloon)
> 	working with page reporting.
> - Change PAGE_REPORTING_DEFAULT_ORDER to -1 as zero is a valid
> 	page order that can be requested.
> 
> Yuvraj Sakshith (3):
>   mm/page_reporting: Allow zero page_reporting_order
>   hv_balloon: Change default page reporting order
>   virtio_balloon: Set pr_dev.order to new default

These look like old stats :)

-- 
Cheers,

David

Re: [PATCH v1 1/4] page_reporting: add PAGE_REPORTING_DEFAULT_ORDER

[David Hildenbrand (Arm)]

On 2/27/26 15:06, Yuvraj Sakshith wrote:
> Drivers can pass order of pages to be reported while
> registering itself. Today, this is a magic number, 0.
> 
> Label this with PAGE_REPORTING_DEFAULT_ORDER and
> check for it when the driver is being registered.

Patch subject: "mm/page_reporting:"

Might want to add "We'll make explicit use of this define in relevant
drivers next."

Acked-by: David Hildenbrand (Arm) <david@kernel.org>


-- 
Cheers,

David

Re: [PATCH v1 2/4] virtio_balloon: set default page reporting order

[David Hildenbrand (Arm)]

On 2/27/26 15:06, Yuvraj Sakshith wrote:
> virtio_balloon page reporting order is set to MAX_PAGE_ORDER implicitly
> as vb->prdev.order is never initialised and is auto-set to zero.
> 
> Explicitly mention usage of default page order by making use of
> PAGE_REPORTING_DEFAULT ORDER fallback value.
> 
> Signed-off-by: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com>

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

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

[David Hildenbrand (Arm)]

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



(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

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

[Yuvraj Sakshith]

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

-       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

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

[Michael Kelley]

From: Yuvraj Sakshith <yuvraj.sakshith@oss.qualcomm.com> 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@oss.qualcomm.com>
> > > ---
> > >  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

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

[David Hildenbrand (Arm)]

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
>>
>> On Fri, Feb 27, 2026 at 09:50:15PM +0100, David Hildenbrand (Arm) wrote:
>>>
>>> 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.

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.

-- 
Cheers,

David

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

[Yuvraj Sakshith]

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
> >>
> >> On Fri, Feb 27, 2026 at 09:50:15PM +0100, David Hildenbrand (Arm) wrote:
> >>>
> >>> 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.
> 
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?

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

And moreover, page_reporting_order can be set by two people. The commandline and
the driver itself. So PAGE_REPORTING_ORDER_NOT_SET can indicate if its set by cmdline
and PAGE_REPORTING_DEFAULT_ORDER can be used by drivers exclusively to "tell" page-reporting
to select the default value for us.

I think what Michael is pointing out is the prevalence of cmdline option over the driver's
request. 

This is not obvious to the reader if we choose to only have one flag IMO :)

Thanks,
Yuvraj
 
> -- 
> Cheers,
> 
> David

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

[David Hildenbrand (Arm)]

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

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

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

[David Hildenbrand (Arm)]

On 3/2/26 09:54, Yuvraj Sakshith wrote:
> On Mon, Mar 02, 2026 at 09:09:13AM +0100, David Hildenbrand (Arm) wrote:
>> On 3/2/26 09:00, Yuvraj Sakshith wrote:
>>> 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;
>>>         }
>>> }
>>>
>>>
>>>
>>> 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?

Maybe PAGE_REPORTING_ORDER_UNSPECIFIED ?

In any case, we should use a single flag for this. Everything else will
be confusing once drivers could use only one of them.

-- 
Cheers,

David

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

[Yuvraj Sakshith]

On Mon, Mar 02, 2026 at 10:18:23AM +0100, David Hildenbrand (Arm) wrote:
> > 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?
> 
> Maybe PAGE_REPORTING_ORDER_UNSPECIFIED ?
> 
> In any case, we should use a single flag for this. Everything else will
> be confusing once drivers could use only one of them.
> 
> -- 
> Cheers,
> 
> David

Sounds good. Thanks for the suggestion.

Thanks,
Yuvraj