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

[Anshuman Khandual]

On 15/07/26 6:29 PM, David Hildenbrand (Arm) wrote:
> 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?

+1
> 
>>  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?

+1
> 
>> +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?

Possibly with a macro PAGE_REPORTING_DELAY_MS_MAX or similar.
> 
>> +
>> +	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.

Agreed.