Message-ID: <0f3b8f9f-a105-4cf9-a0df-bcca66f88c9b@linux.alibaba.com>
Date: Wed, 25 Jun 2025 14:55:19 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Dev Jain <dev.jain@....com>, Hugh Dickins <hughd@...gle.com>
Cc: akpm@...ux-foundation.org, david@...hat.com, ziy@...dia.com,
 lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, npache@...hat.com,
 ryan.roberts@....com, baohua@...nel.org, zokeefe@...gle.com,
 shy828301@...il.com, usamaarif642@...il.com, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/2] fix MADV_COLLAPSE issue if THP settings are
 disabled



On 2025/6/25 14:49, Dev Jain wrote:
> 
> On 25/06/25 11:56 am, Baolin Wang wrote:
>>
>>
>> On 2025/6/25 13:53, Hugh Dickins wrote:
>>> On Wed, 25 Jun 2025, Baolin Wang wrote:
>>>
>>>> When invoking thp_vma_allowable_orders(), if the TVA_ENFORCE_SYSFS 
>>>> flag is not
>>>> specified, we will ignore the THP sysfs settings. Whilst it makes 
>>>> sense for the
>>>> callers who do not specify this flag, it creates a odd and 
>>>> surprising situation
>>>> where a sysadmin specifying 'never' for all THP sizes still 
>>>> observing THP pages
>>>> being allocated and used on the system. And the MADV_COLLAPSE is an 
>>>> example of
>>>> such a case, that means it will not set TVA_ENFORCE_SYSFS when calling
>>>> thp_vma_allowable_orders().
>>>>
>>>> As we discussed in the previous thread [1], the MADV_COLLAPSE will 
>>>> ignore
>>>> the system-wide anon/shmem THP sysfs settings, which means that even 
>>>> though
>>>> we have disabled the anon/shmem THP configuration, MADV_COLLAPSE 
>>>> will still
>>>> attempt to collapse into a anon/shmem THP. This violates the rule we 
>>>> have
>>>> agreed upon: never means never.
>>>>
>>>> For example, system administrators who disabled THP everywhere must 
>>>> indeed very
>>>> much not want THP to be used for whatever reason - having individual 
>>>> programs
>>>> being able to quietly override this is very surprising and likely to 
>>>> cause headaches
>>>> for those who desire this not to happen on their systems.
>>>>
>>>> This patch set will address the MADV_COLLAPSE issue.
>>>>
>>>> Test
>>>> ====
>>>> 1. Tested the mm selftests and found no regressions.
>>>> 2. With toggling different Anon mTHP settings, the allocation and 
>>>> madvise collapse for
>>>> anonymous pages work well.
>>>> 3. With toggling different shmem mTHP settings, the allocation and 
>>>> madvise collapse for
>>>> shmem work well.
>>>> 4. Tested the large order allocation for tmpfs, and works as expected.
>>>>
>>>> [1] https://lore.kernel.org/all/1f00fdc3- 
>>>> a3a3-464b-8565-4c1b23d34f8d@...ux.alibaba.com/
>>>>
>>>> Changes from v3:
>>>>   - Collect reviewed tags. Thanks.
>>>>   - Update the commit message, per David.
>>>>
>>>> Changes from v2:
>>>>   - Update the commit message and cover letter, per Lorenzo. Thanks.
>>>>   - Simplify the logic in thp_vma_allowable_orders(), per Lorenzo 
>>>> and David. Thanks.
>>>>
>>>> Changes from v1:
>>>>   - Update the commit message, per Zi.
>>>>   - Add Zi's reviewed tag. Thanks.
>>>>   - Update the shmem logic.
>>>>
>>>> Baolin Wang (2):
>>>>    mm: huge_memory: disallow hugepages if the system-wide THP sysfs
>>>>      settings are disabled
>>>>    mm: shmem: disallow hugepages if the system-wide shmem THP sysfs
>>>>      settings are disabled
>>>>
>>>>   include/linux/huge_mm.h                 | 51 +++++++++++++++++ 
>>>> +-------
>>>>   mm/shmem.c                              |  6 +--
>>>>   tools/testing/selftests/mm/khugepaged.c |  8 +---
>>>>   3 files changed, 43 insertions(+), 22 deletions(-)
>>>>
>>>> -- 
>>>> 2.43.5
>>>
>>> Sorry for chiming in so late, after so much effort: but I beg you,
>>> please drop these.
>>
>> Thanks Hugh for your input. (yes, we put in a lot of effort on 
>> discussion and testing:( ).
>>
>>> I did not want to get into a fight, and had been hoping a voice of
>>> reason would come from others, before I got around to responding.
>>>
>>> And indeed Ryan understood correctly at the start; and he, Usama
>>> and Barry, perhaps others I've missed, have raised appropriate
>>> concerns but not prevailed.
>>>
>>> If we're sloganeering, I much prefer "never break userspace" to
>>> "never means never", attractive though that over-simplification is.
>>
>> Yes, agree. we should not break userspace, however, I suspect whether 
>> this can really break userspace. We can set '/sys/kernel/mm/ 
>> transparent_hugepage/enabled' to 'madvise' to allow MADV_COLLAPSE. 
>> Additionally, I really doubt that when the system-wide THP settings 
>> are set to 'never', userspace would still expect to collapse into THP 
>> using MADV_COLLAPSE.
> 
> After this patch, will a user still be able to use MADV_COLLAPSE and 
> ensure no interference from khugepaged?

I think so. Becuase khugepaged will still check VM_HUGEPAGE if we set 
'/sys/kernel/mm/transparent_hugepage/enabled' to 'madvise'.

>> Moreover, what makes this issue particularly frustrating is that when 
>> we introduce mTHP collapse[1], MADV_COLLAPSE complicates matters 
>> further. That is, when the system only enables 64K mTHP, MADV_COLLAPSE 
>> still allows collapsing into PMD-sized THP. This really breaks the 
>> user's settings.
> 
> This issue will still be there without this patch right?

NO. Will fix this issue. After this patch, MADV_COLLAPSE can not 
continue to collapse PMD-sized THP if the system only enables 64K mTHP.

>> [1] https://lore.kernel.org/all/20250515032226.128900-1- 
>> npache@...hat.com/
>>
>>> Seldom has a feature been so thorougly documented as MADV_COLLAPSE,
>>> in its 6.1 commits and in the "man 2 madvise" page: which are
>>> explicit about MADV_COLLAPSE providing a way to get THPs where the
>>> sysfs setting governing automatic behaviour does not insert them.
>>>
>>> We would all prefer a less messy world of THP tunables.  I certainly
>>> find plenty to dislike there too; and wish that a less assertive name
>>> than "never" had been chosen originally for the default off position.
>>>
>>> But please don't break the accepted and documented behaviour of
>>> MADV_COLLAPSE now.
>>>
>>> If you want to exclude all possibility of THPs, then please use the
>>> prctl(PR_SET_THP_DISABLE); or shmem_enabled=deny (I think it was me
>>> who insisted that be respected by MADV_COLLAPSE back then).
>>
>> Yes, that will prevent MADV_COLLAPSE.
>>
>>> Add a "deny" option to /sys/kernel/mm/transparent_hugepage/enabled
>>> if you like.  (But in these days of filesystem large folios, adding
>>> new protections against them seems a few years late.)
>>>
>>> If Andrew decides that these patches should go in, then I'll have to
>>> scrutinize them more carefully than I've done so far: but currently
>>> I'm hoping to avoid that.
>>>
>>> Hugh
>>

Powered by blists - more mailing lists