Re: [PATCH mm-new 1/1] mm/khugepaged: Factor out common logic in [scan,alloc]_sleep_millisecs_store()

Leon Hwang
2025-10-20T15:32:54Z

On 2025/10/20 21:53, Lorenzo Stoakes wrote:
> On Mon, Oct 20, 2025 at 07:53:50PM +0800, Leon Hwang wrote:
>> Both scan_sleep_millisecs_store() and alloc_sleep_millisecs_store()
>> perform the same operations: parse the input value, update their
>> respective sleep interval, reset khugepaged_sleep_expire, and wake up
>> the khugepaged thread.
>>
>> Factor out this duplicated logic into a helper function
>> __sleep_millisecs_store(), and simplify both store functions.
>>
>> No functional change intended.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>
> Thanks this is a decent cleanup, with the nit(s0 below addressed LGTM, so:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>

Thanks for the review, Lorenzo.

>> ---
>> mm/khugepaged.c | 29 ++++++++++++++---------------
>> 1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 0341c3d13e9e..0b7915015e9e 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -129,9 +129,8 @@ static ssize_t scan_sleep_millisecs_show(struct kobject *kobj,
>> return sysfs_emit(buf, "%u\n", khugepaged_scan_sleep_millisecs);
>> }
>>
>> -static ssize_t scan_sleep_millisecs_store(struct kobject *kobj,
>> - struct kobj_attribute *attr,
>> - const char *buf, size_t count)
>> +static ssize_t __sleep_millisecs_store(const char *buf, size_t count,
>> + unsigned int *millisecs)
>> {
>> unsigned int msecs;
>> int err;
>> @@ -140,12 +139,21 @@ static ssize_t scan_sleep_millisecs_store(struct kobject *kobj,
>> if (err)
>> return -EINVAL;
>>
>> - khugepaged_scan_sleep_millisecs = msecs;
>> + *millisecs = msecs;
>> khugepaged_sleep_expire = 0;
>> wake_up_interruptible(&khugepaged_wait);
>>
>> return count;
>> }
>> +
>> +static ssize_t scan_sleep_millisecs_store(struct kobject *kobj,
>> + struct kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + unsigned int *millisecs = &khugepaged_scan_sleep_millisecs;
>> +
>> + return __sleep_millisecs_store(buf, count, millisecs);
>
> I think this'd be much clearer as:
> > return __sleep_millisecs_store(buf, count,
&khugepaged_alloc_scan_millisecs);
>
>> +}
>> static struct kobj_attribute scan_sleep_millisecs_attr =
>> __ATTR_RW(scan_sleep_millisecs);
>>
>> @@ -160,18 +168,9 @@ static ssize_t alloc_sleep_millisecs_store(struct kobject *kobj,
>> struct kobj_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - unsigned int msecs;
>> - int err;
>> -
>> - err = kstrtouint(buf, 10, &msecs);
>> - if (err)
>> - return -EINVAL;
>> + unsigned int *millisecs = &khugepaged_alloc_sleep_millisecs;
>>
>> - khugepaged_alloc_sleep_millisecs = msecs;
>> - khugepaged_sleep_expire = 0;
>> - wake_up_interruptible(&khugepaged_wait);
>> -
>> - return count;
>> + return __sleep_millisecs_store(buf, count, millisecs);
>> }
>
> I think this'd be much clearer as:
>
> return __sleep_millisecs_store(buf, count, &khugepaged_alloc_sleep_millisecs);
>

Indeed, your suggestion makes the code clearer.

I'll update both occurrences in the next revision.

Thanks,
Leon

>> static struct kobj_attribute alloc_sleep_millisecs_attr =
>> __ATTR_RW(alloc_sleep_millisecs);
>> --
>> 2.51.0
>>