Re: [PATCH 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()

[ranxiaokai627]

>CC Breno and Lorenzo
>
>On 5/12/26 8:05 PM, ranxiaokai627@xxxxxxx wrote:
>> From: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
>> 
>> Inspired by commit 82d9ff648c6c ("mm: huge_memory: refactor
>> anon_enabled_store() with set_anon_enabled_mode()"), refactor
>> thpsize_shmem_enabled_store() using sysfs_match_string().
>> This eliminates the duplicated spin_lock/unlock(), set/clear_bit(),
>> calls across all branches, reducing code duplication.
>> 
>> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
>> all test cases passed.
>> 
>> Signed-off-by: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
>> ---
>
>Thanks for doing this.
>
>>   mm/shmem.c | 88 ++++++++++++++++++++++++++----------------------------
>>   1 file changed, 43 insertions(+), 45 deletions(-)
>> 
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 3b5dc21b323c..0cc7872cc576 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -5526,6 +5526,29 @@ static ssize_t shmem_enabled_store(struct kobject *kobj,
>>   struct kobj_attribute shmem_enabled_attr = __ATTR_RW(shmem_enabled);
>>   static DEFINE_SPINLOCK(huge_shmem_orders_lock);
>>   
>
>We are already under the lock, so you can use non-atomic functions like 
>commit 82d9ff648c6c does: __test_and_set_bit/__test_and_clear_bit.
>
>> +	spin_unlock(&huge_shmem_orders_lock);
>>   
>> -	if (ret > 0) {
>> -		int err = start_stop_khugepaged();
>> +	err = start_stop_khugepaged();
>> +	if (err)
>
>Moreover, I think we can follow commit 82d9ff648c6c's approach: if 
>nothing changed, we don't need to call start_stop_khugepaged().

Thanks for the review.
Yes, indeed, We can further optimize this by following the approach
in commit 82d9ff648c6c. 

It seems odd to still call set_recommended_min_free_kbytes() when the 
mode hasn't changed. Maybe this is to handle the case where the user
modified /proc/sys/vm/min_free_kbytes in this timewindow?

I'll follow the existing logic and send a v2.