Re: [RFC PATCH] mm/page_reporting: Add page_reporting_delay sysctl

[David Hildenbrand (Arm)]

On 7/14/26 19:14, pratmal@xxxxxxxxxx wrote:
> From: Pratyush Mallick <pratmal@xxxxxxxxxx>
> 
> Currently, the free page reporting daemon uses a hardcoded delay of
> (2 HZ) between reporting intervals. While this is a reasonable
> default, it lacks the flexibility to adapt to varying guest workloads.
> 
> A low delay allows aggressive memory reclamation, returning unused
> pages to the hypervisor as quickly as possible. However, during spiky
> allocation/free churn, this immediate reporting can lead to a severe
> performance penalty (nested page faults) as the guest re-allocates memory
> that the hypervisor has just unmapped. In these scenarios, there is benefit
> from increasing the delay to batch free pages over a longer window,
> absorbing the churn without hypercall and re-fault overhead.
> 
> This patch refactors the delay into a dynamically tunable sysctl,
> /proc/sys/vm/page_reporting_delay, measured in milliseconds. The value
> defaults to 2000ms to precisely match the original (2 HZ) behavior.
> If the sysctl is modified across reporting windows, the sysctl handler
> immediately issues a mod_delayed_work() to honor the new configuration
> without waiting for the prior timeout to lapse.
> 
> Signed-off-by: Pratyush Mallick <pratmal@xxxxxxxxxx>
> ---
> 
> Sending this as an RFC to get thoughts on exposing this delay as a sysctl. 
>                                                                            
> Benchmark Results:
> We kill a process allocated with 10GB memory within the VM and mesure the
> time it takes to report all the memory to host as well the delay in
> initiating the reporting.
>  
> default(2s delay): https://drive.google.com/file/d/1Ouxm_raj4xPNthc_ryk0Mdbo-c_zXKiP/view?usp=sharing
> Tuned to 0s delay: https://drive.google.com/file/d/1mk58LPiYgIF4Yk6kmX5JHplhrwIw0NbC/view?usp=sharing

[...]

Hi!

>  
>  #include "page_reporting.h"
> @@ -47,15 +48,44 @@ MODULE_PARM_DESC(page_reporting_order, "Set page reporting order");
>   */
>  EXPORT_SYMBOL_GPL(page_reporting_order);
>  
> -#define PAGE_REPORTING_DELAY	(2 * HZ)
> -static struct page_reporting_dev_info __rcu *pr_dev_info __read_mostly;
> -

Why are you moving that?

>  enum {
>  	PAGE_REPORTING_IDLE = 0,
>  	PAGE_REPORTING_REQUESTED,
>  	PAGE_REPORTING_ACTIVE
>  };
>  
> +static unsigned int page_reporting_delay = 2000;

Maybe "2 * MSEC_PER_SEC;"

Would we want to call that page_reporting_delay_ms to make it clearer what we
are dealing with?

> +static struct page_reporting_dev_info __rcu *pr_dev_info __read_mostly;
> +
> +static int page_reporting_delay_sysctl(const struct ctl_table *table, int write,
> +				       void *buffer, size_t *lenp, loff_t *ppos)

We prefer two tabs here in MM land.

> +{
> +	int ret;
> +	struct page_reporting_dev_info *prdev;
> +
> +	ret = proc_dointvec(table, write, buffer, lenp, ppos);
> +	if (ret < 0 || !write)
> +		return ret;

Would we want to cap it at reasonable values?

> +
> +	rcu_read_lock();
> +	prdev = rcu_dereference(pr_dev_info);
> +	if (prdev && atomic_read(&prdev->state) == PAGE_REPORTING_REQUESTED)
> +		mod_delayed_work(system_wq, &prdev->work, msecs_to_jiffies(page_reporting_delay));
> +	rcu_read_unlock();

Is that really required? Seems unnecessary given that we expect something in the
range of a couple of seconds max.

-- 
Cheers,

David