Re: [PATCH v2 0/2] fix MADV_COLLAPSE issue if THP settings are disabled

[Baolin Wang]

On 2025/6/7 20:28, Lorenzo Stoakes wrote:
> Before I get into technical criticism, to be clear - thank you very much
> for doing this :) I'm just getting into details as to the implementation,
> but am a fan of this change and consider it important.
>
> On Thu, Jun 05, 2025 at 04:00:57PM +0800, Baolin Wang wrote:
> > 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. This patch set will address this issue.
>
> Hm this cover letter could be expanded upon quite a bit - you are doing a
> lot here and it's not only MADV_COLLAPSE, more a general change.
>
> I'd mention that, even when TVA_ENFORCE_SYSFS is not set, callers checking
> THP order validity will not be able to specify THP orders that are either
> specifically marked as 'never' or set to 'inherit' and the global hugepage
> mode is 'never'.
>
> Then say something like 'importantly, this changes alters the madvise(...,
> MADV_COLLAPSE) call, which previously would collapse ranges into huge pages
> even if THP was set to never. This corrects this behaviour'.
>
> I suspect you are unable to write sensible tests here given the need to
> manipulate sysfs (though perhaps worth quickly looking at
> tools/testing/selftests/mm/khugepaged.c, transhuge-stress.c, run_vmtests.sh
> to see), but it'd be at least useful for you to give details here of how
> you have tested this and ensured it functions correctly.
>
> It might also be worth giving a quick justification, i.e. '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'.

Ah, missed this comment. Good suggestion, I will update the cover 
letter. Thanks.