Re: [PATCH] mm, thp: display [never] for defrag when THP is set to never

[David Hildenbrand <david@redhat.com>]

On 26.02.24 12:48, David Hildenbrand wrote:
> On 26.02.24 12:22, Liu Song wrote:
>>
>> 在 2024/2/26 16:54, David Hildenbrand 写道:
>>> On 23.02.24 12:01, Liu Song wrote:
>>>>
>>>> 在 2024/2/22 20:14, David Hildenbrand 写道:
>>>>> On 22.02.24 12:53, Yuanhe Shu wrote:
>>>>>> When transparent_hugepage is set to never by cmdline or echo, defrag
>>>>>> still show what it used to be and can be modified which makes user
>>>>>> confusing whether defrag would take effect.
>>>>>>
>>>>>> Actually if transparent_hugepage is set to never, defrag will not take
>>>>>> effect. Just Display never and remain unchangeable to for defrag when
>>>>>> transparent_hugepage is set to never.
>>>>>>
>>>>>> Suggested-by: Liu Song <liusong@linux.alibaba.com>
>>>>>> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
>>>>>> ---
>>>>>
>>>>> No, I don't think we want such a dependency between both options.
>>>>>
>>>>> You might just end up breaking existing scripts (enable defrag before
>>>>> enabling THP) for no good reason.
>>>>>
>>>> In certain situations where khugepaged_thread is NULL, it would be more
>>>> reasonable for the value of
>>>> /sys/kernel/mm/transparent_hugepage/khugepaged/defrag to be 0. The patch
>>>> should include a fix for this case.
>>>
>>> Why?
>>>
>>> We have a bunch of THP toggles. They reside in
>>> "/sys/kernel/mm/transparent_hugepage/", indicating that they are THP
>>> specific.
>>>
>>> Some of them are only in effect if some other toggles are set.
>>>
>>> That is very common practice.
>>>
>>> If you think something could be confusing, maybe clarify the doc? I
>>> don't immediately see why any code changes are required, really.
>>
>> We should explain this in the documentation, but to be honest, many
>> people don't read the documentation, and even after we explicitly
>> disable THP with transparent_hugepage=never, khugepaged/defrag is still
>> set to 1, and users come asking why it's still defragging. We can't
>> expect all users to have technical expertise, or to diligently read
>> through the documentation; it would obviously be best if we could avoid
>> user confusion altogether.
> 
> Then tell your users if a feature is enabled

s/enabled/disabled/
s/1on1/101/

Time for a break :D

Anyhow, there has to be a pretty good reason to change the semantics of 
these toggles that have been around forever. "users could be confused" 
is a not sufficient.

-- 
Cheers,

David / dhildenb