[PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[Gavin Shan]

The inappropriate settings can be accepted by "enabled" and "defrag"
interface, as the following example shows. This series reworks the
related functions and fixes the issue by the way.

   # cat /sys/kernel/mm/transparent_hugepage/enabled
     always [madvise] never
   # echo neveroff > /sys/kernel/mm/transparent_hugepage/enabled
   # cat /sys/kernel/mm/transparent_hugepage/enabled
     always madvise [never]

Gavin Shan (3):
  mm: Rework {enabled,defrag}_store()
  mm: Rework {enabled,defrag}_show()
  mm: Rework setup_transparent_hugepage()

 mm/huge_memory.c | 216 +++++++++++++++++++++++++++--------------------
 1 file changed, 125 insertions(+), 91 deletions(-)

-- 
2.23.0

Re: [PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[Michal Hocko]

On Thu 30-01-20 13:24:38, Gavin Shan wrote:
> The inappropriate settings can be accepted by "enabled" and "defrag"
> interface, as the following example shows. This series reworks the
> related functions and fixes the issue by the way.

Do we really care? This is a root only interface and we do expect root
to know how to use the interface.

>    # cat /sys/kernel/mm/transparent_hugepage/enabled
>      always [madvise] never
>    # echo neveroff > /sys/kernel/mm/transparent_hugepage/enabled
>    # cat /sys/kernel/mm/transparent_hugepage/enabled
>      always madvise [never]
> 
> Gavin Shan (3):
>   mm: Rework {enabled,defrag}_store()
>   mm: Rework {enabled,defrag}_show()
>   mm: Rework setup_transparent_hugepage()
> 
>  mm/huge_memory.c | 216 +++++++++++++++++++++++++++--------------------
>  1 file changed, 125 insertions(+), 91 deletions(-)
> 
> -- 
> 2.23.0
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[Vlastimil Babka]

On 1/30/20 8:49 AM, Michal Hocko wrote:
> On Thu 30-01-20 13:24:38, Gavin Shan wrote:
>> The inappropriate settings can be accepted by "enabled" and "defrag"
>> interface, as the following example shows. This series reworks the
>> related functions and fixes the issue by the way.
> 
> Do we really care? This is a root only interface and we do expect root
> to know how to use the interface.

Also I think David already solved even this case with sysfs_streq() that
Andrew suggested:

https://lore.kernel.org/linux-mm/alpine.DEB.2.21.2001171411020.56385@chino.kir.corp.google.com/

>>    # cat /sys/kernel/mm/transparent_hugepage/enabled
>>      always [madvise] never
>>    # echo neveroff > /sys/kernel/mm/transparent_hugepage/enabled
>>    # cat /sys/kernel/mm/transparent_hugepage/enabled
>>      always madvise [never]
>>
>> Gavin Shan (3):
>>   mm: Rework {enabled,defrag}_store()
>>   mm: Rework {enabled,defrag}_show()
>>   mm: Rework setup_transparent_hugepage()
>>
>>  mm/huge_memory.c | 216 +++++++++++++++++++++++++++--------------------
>>  1 file changed, 125 insertions(+), 91 deletions(-)
>>
>> -- 
>> 2.23.0
>>
> 

Re: [PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[Gavin Shan]

On 1/30/20 6:49 PM, Michal Hocko wrote:
> On Thu 30-01-20 13:24:38, Gavin Shan wrote:
>> The inappropriate settings can be accepted by "enabled" and "defrag"
>> interface, as the following example shows. This series reworks the
>> related functions and fixes the issue by the way.
> 
> Do we really care? This is a root only interface and we do expect root
> to know how to use the interface.
> 

I'm not sure actually, but the behavior (suffix matching) isn't well documented
in admin-guide/mm/transhuge.rst at least. I think it's not bad to enforce a
full matching here because even root user can input something wrong :)

Thanks,
Gavin.

>>     # cat /sys/kernel/mm/transparent_hugepage/enabled
>>       always [madvise] never
>>     # echo neveroff > /sys/kernel/mm/transparent_hugepage/enabled
>>     # cat /sys/kernel/mm/transparent_hugepage/enabled
>>       always madvise [never]
>>
>> Gavin Shan (3):
>>    mm: Rework {enabled,defrag}_store()
>>    mm: Rework {enabled,defrag}_show()
>>    mm: Rework setup_transparent_hugepage()
>>
>>   mm/huge_memory.c | 216 +++++++++++++++++++++++++++--------------------
>>   1 file changed, 125 insertions(+), 91 deletions(-)
>>
>> -- 
>> 2.23.0
>>
> 

Re: [PATCH 0/3] mm: Rework {enabled,defrag}_{show,store}

[Gavin Shan]

On 1/30/20 8:46 PM, Vlastimil Babka wrote:
> On 1/30/20 8:49 AM, Michal Hocko wrote:
>> On Thu 30-01-20 13:24:38, Gavin Shan wrote:
>>> The inappropriate settings can be accepted by "enabled" and "defrag"
>>> interface, as the following example shows. This series reworks the
>>> related functions and fixes the issue by the way.
>>
>> Do we really care? This is a root only interface and we do expect root
>> to know how to use the interface.
> 
> Also I think David already solved even this case with sysfs_streq() that
> Andrew suggested:
> 
> https://lore.kernel.org/linux-mm/alpine.DEB.2.21.2001171411020.56385@chino.kir.corp.google.com/
> 

Thanks for the linker. I didn't notice there already have a patch to address
the issue. Also, I perhaps post patch to improve the extendibility on top of
David's patch if you agree. Otherwise, it can be ignored.

By the way, it's nice to know sysfs_streq().

Thanks,
Gavin

>>>     # cat /sys/kernel/mm/transparent_hugepage/enabled
>>>       always [madvise] never
>>>     # echo neveroff > /sys/kernel/mm/transparent_hugepage/enabled
>>>     # cat /sys/kernel/mm/transparent_hugepage/enabled
>>>       always madvise [never]
>>>
>>> Gavin Shan (3):
>>>    mm: Rework {enabled,defrag}_store()
>>>    mm: Rework {enabled,defrag}_show()
>>>    mm: Rework setup_transparent_hugepage()
>>>
>>>   mm/huge_memory.c | 216 +++++++++++++++++++++++++++--------------------
>>>   1 file changed, 125 insertions(+), 91 deletions(-)
>>>
>>> -- 
>>> 2.23.0
>>>
>>
>