Message-ID: <6502bbb7-e8b3-4520-9547-823207119061@lucifer.local>
Date: Thu, 15 May 2025 14:55:28 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Usama Arif <usamaarif642@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, david@...hat.com,
        linux-mm@...ck.org, hannes@...xchg.org, shakeel.butt@...ux.dev,
        riel@...riel.com, ziy@...dia.com, laoar.shao@...il.com,
        baolin.wang@...ux.alibaba.com, Liam.Howlett@...cle.com,
        npache@...hat.com, ryan.roberts@....com, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH 0/6] prctl: introduce PR_SET/GET_THP_POLICY

On Thu, May 15, 2025 at 02:33:29PM +0100, Usama Arif wrote:
> This allows to change the THP policy of a process, according to the value
> set in arg2, all of which will be inherited during fork+exec:

This is pretty confusing.

It should be something like 'add a new prctl() option that allows...' etc.

> - PR_THP_POLICY_DEFAULT_HUGE: This will set the MMF2_THP_VMA_DEFAULT_HUGE
>   process flag which changes the default of new VMAs to be VM_HUGEPAGE. The
>   call also modifies all existing VMAs that are not VM_NOHUGEPAGE
>   to be VM_HUGEPAGE.

This is referring to implementation detail that doesn't matter for an overview,
just add a summary here e.g.

PR_THP_POLICY_DEFAULT_HUGE - set VM_HUGEPAGE flag in all VMAs by default,
including after fork/exec, ignoring global policy.

PR_THP_POLICY_DEFAULT_NOHUGE - clear VM_HUGEPAGE flag in all VMAs by default,
including after fork/exec, ignoring global policy.

PR_THP_POLICY_DEFAULT_SYSTEM - Eliminate any policy set above.

>   This allows systems where the global policy is set to "madvise"
>   to effectively have THPs always for the process. In an environment
>   where different types of workloads are stacked on the same machine
>   whose global policy is set to "madvise", this will allow workloads
>   that benefit from always having hugepages to do so, without regressing
>   those that don't.

So does this just ignore and override the global policy? I'm not sure I'm
comfortable with that.

What about if the the policy is 'never'? Does this override that? That seems
completely wrong.

> - PR_THP_POLICY_DEFAULT_NOHUGE: This will set the MMF2_THP_VMA_DEFAULT_NOHUGE
>   process flag which changes the default of new VMAs to be VM_NOHUGEPAGE.
>   The call also modifies all existing VMAs that are not VM_HUGEPAGE
>   to be VM_NOHUGEPAGE.
>   This allows systems where the global policy is set to "always"
>   to effectively have THPs on madvise only for the process. In an
>   environment where different types of workloads are stacked on the
>   same machine whose global policy is set to "always", this will allow
>   workloads that benefit from having hugepages on an madvise basis only
>   to do so, without regressing those that benefit from having hugepages
>   always.

Wait, so 'no huge' means 'madvise'? What? This is confusing.

> - PR_THP_POLICY_DEFAULT_SYSTEM: This will clear the MMF2_THP_VMA_DEFAULT_HUGE
>   and MMF2_THP_VMA_DEFAULT_NOHUGE process flags.
>
> These patches are required in rolling out hugepages in hyperscaler
> configurations for workloads that benefit from them, where workloads are
> stacked anda single THP global policy is likely to be used across the entire
> fleet, and prctl will help override it.

I don't understand this justification whatsoever. What does 'stacked' mean? And
you're not justifying why you'd override the policy?

This series has no actual justificaiton here at all? You really need to provide one.

>
> v1->v2:

Where was the v1? Is it [0]?

This seems like a massive change compared to that series?

You've renamed it and not referenced the old series, please make sure you link
it or somehow let somebody see what this is against, because it makes review
difficult.

[0]: https://lore.kernel.org/linux-mm/20250507141132.2773275-1-usamaarif642@gmail.com/

> - change from modifying the THP decision making for the process, to modifying
>   VMA flags only. This prevents further complicating the logic used to
>   determine THP order (Thanks David!)
> - change from using a prctl per policy change to just using PR_SET_THP_POLICY
>   and arg2 to set the policy. (Zi Yan)
> - Introduce PR_THP_POLICY_DEFAULT_NOHUGE and PR_THP_POLICY_DEFAULT_SYSTEM
> - Add selftests and documentation.
>
> Usama Arif (6):
>   prctl: introduce PR_THP_POLICY_DEFAULT_HUGE for the process
>   prctl: introduce PR_THP_POLICY_DEFAULT_NOHUGE for the process
>   prctl: introduce PR_THP_POLICY_SYSTEM for the process
>   selftests: prctl: introduce tests for PR_THP_POLICY_DEFAULT_NOHUGE
>   selftests: prctl: introduce tests for PR_THP_POLICY_DEFAULT_HUGE
>   docs: transhuge: document process level THP controls
>
>  Documentation/admin-guide/mm/transhuge.rst    |  40 +++
>  include/linux/huge_mm.h                       |   4 +
>  include/linux/mm_types.h                      |  14 +
>  include/uapi/linux/prctl.h                    |   6 +
>  kernel/fork.c                                 |   1 +
>  kernel/sys.c                                  |  35 +++
>  mm/huge_memory.c                              |  56 ++++
>  mm/vma.c                                      |   2 +
>  tools/include/uapi/linux/prctl.h              |   6 +
>  .../trace/beauty/include/uapi/linux/prctl.h   |   6 +
>  tools/testing/selftests/prctl/Makefile        |   2 +-
>  tools/testing/selftests/prctl/thp_policy.c    | 286 ++++++++++++++++++
>  12 files changed, 457 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/prctl/thp_policy.c
>
> --
> 2.47.1
>

Powered by blists - more mailing lists