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

[pratmal@google.com]

On 7/16/26 02:41, Anshuman Khandual wrote:
> On 15/07/26 6:29 PM, David Hildenbrand (Arm) wrote:
>> -#define PAGE_REPORTING_DELAY	(2 * HZ)
>> -static struct page_reporting_dev_info __rcu *pr_dev_info __read_mostly;
>> -
>> Why are you moving that?
> 
> +1

I wanted the new definitions grouped together, and the sysctl handler has
to sit below the enum so it can see PAGE_REPORTING_REQUESTED. But that
only constrains where the handler goes, not the declaration. I'll leave
pr_dev_info where it was.

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

Agreed on both. I'll rename the sysctl to page_reporting_delay_ms as well
in v2.

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

Thanks. Will fix this.

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

Yes. Will fix this in v2.

Will use  proc_douintvec_minmax() with a floor and a ceiling (using a 
new PAGE_REPORTING_DELAY_MS_MAX macro).

>> +	rcu_read_lock();
>> +	prdev = rcu_dereference(pr_dev_info);
>> +	if (prdev && atomic_read(&prdev->state) == PAGE_REPORTING_REQUESTED)
>> +		mod_delayed_work(...);
>> +	rcu_read_unlock();
>>
>> Is that really required? Seems unnecessary given that we expect something in the
>> range of a couple of seconds max.
> 
> Agreed.

Our use case is that the delay isn't static. A daemon in the guest watches
for host memory pressure and drops the delay to expedite reporting, then
raises it again once the pressure clears. The delay can be tens of seconds
when we're absorbing churn, so waiting out the current window defeats the
purpose of the dynamic adjustment. Apologies, I should have clarified this 
in the original commit message.

I'm happy to drop it or decouple this triggering behavior from the tuning
behavior. We could probably create a separate sysctl to manually trigger 
the reporting instead.

Thanks to both of you for the review.

Pratyush Mallick.