Andrew -

I hope the explanation below resolves your query about the header include
(in [0]), let me know if doing this as a series like this works (we need to
enforce the ordering here).

Thanks!

[0]: 20250514153648.598bb031a2e498b1ac505b60@linux-foundation.org



Currently, when somebody attempts to set MADV_NOHUGEPAGE on a system that
does not enable CONFIG_TRANSPARENT_HUGEPAGE the confguration option, this
results in an -EINVAL error arising.

This doesn't really make sense, as to do so is essentially a no-op.

Additionally, the semantics of setting VM_[NO]HUGEPAGE in any case are such
that, should the attribute not apply, nothing will be done.

It therefore makes sense to simply make this operation a noop.

However, a fly in the ointment is that, in order to do so, we must check
against the MADV_NOHUGEPAGE constant. In doing so, we encounter two rather
annoying issues.

The first is that the usual include we would import to get hold of
MADV_NOHUGEPAGE, linux/mman.h, results in a circular dependency:

* If something includes linux/mman.h, we in turn include linux/mm.h prior
  to declaring MADV_NOHUGEPAGE.
* This then, in turn, includes linux/huge_mm.h.
* linux/huge_mm.h declares hugepage_madvise(), which then tries to
  reference MADV_NOHUGEPAGE, and the build fails.

This can be reached in other ways too.

So we work around this by including uapi/asm/mman.h instead, which allows
us to keep hugepage_madvise() inline.

The second issue is that the s390 arch declares PROT_NONE as a value in the
enum prot_type enumeration.

By updating the include in linux/huge_mm.h, we pull in the PROT_NONE
declaration (unavoidably, this is ultimately in
uapi/asm-generic/mman-common.h alongside MADV_NOHUGEPAGE), which collides
with the enumeration value.

To resolve this, we rename PROT_NONE to PROT_TYPE_DUMMY.

The ordering of these patches is critical, the s390 patch must be applied
prior to the MADV_NOHUGEPAGE patch, and therefore the two patches are sent
as a series.

v1:
* Place patches in series.
* Correct typo in comment as per James.

previous patches:
huge_mm.h patch - https://lore.kernel.org/all/20250508-madvise-nohugepage-noop-without-thp-v1-1-e7ceffb197f3@kuka.com/
s390 patch - https://lore.kernel.org/all/20250514163530.119582-1-lorenzo.stoakes@oracle.com/

Ignacio Moreno Gonzalez (1):
  mm: madvise: make MADV_NOHUGEPAGE a no-op if !THP

Lorenzo Stoakes (1):
  KVM: s390: rename PROT_NONE to PROT_TYPE_DUMMY

 arch/s390/kvm/gaccess.c | 8 ++++----
 include/linux/huge_mm.h | 5 +++++
 2 files changed, 9 insertions(+), 4 deletions(-)

--
2.49.0

Andrew -

OK, I realise there's an issue here with patch 2/2. We're not accounting
for the fact that madvise() will reject this _anyway_ because
madvise_behavior_valid() will reject it.

I've tried to be especially helpful here to aid Ignacio in his early
contributions, but I think it's best now (if you don't mind Igancio) for me
to figure out a better solution after the merge window.

We're late in the cycle now so I will just resend the 1st patch (for s390)
separately if you're happy to take that for 6.16? It's a simple rename of
an entirely static identifier so should present no risk, and is approved by
the arch maintainers who have also agreed for it to come through the mm
tree.

Apologies for the mess!

Cheers, Lorenzo

On Thu, May 15, 2025 at 09:15:44PM +0100, Lorenzo Stoakes wrote:
> Andrew -
>
> I hope the explanation below resolves your query about the header include
> (in [0]), let me know if doing this as a series like this works (we need to
> enforce the ordering here).
>
> Thanks!
>
> [0]: 20250514153648.598bb031a2e498b1ac505b60@linux-foundation.org
>
>
>
> Currently, when somebody attempts to set MADV_NOHUGEPAGE on a system that
> does not enable CONFIG_TRANSPARENT_HUGEPAGE the confguration option, this
> results in an -EINVAL error arising.
>
> This doesn't really make sense, as to do so is essentially a no-op.
>
> Additionally, the semantics of setting VM_[NO]HUGEPAGE in any case are such
> that, should the attribute not apply, nothing will be done.
>
> It therefore makes sense to simply make this operation a noop.
>
> However, a fly in the ointment is that, in order to do so, we must check
> against the MADV_NOHUGEPAGE constant. In doing so, we encounter two rather
> annoying issues.
>
> The first is that the usual include we would import to get hold of
> MADV_NOHUGEPAGE, linux/mman.h, results in a circular dependency:
>
> * If something includes linux/mman.h, we in turn include linux/mm.h prior
>   to declaring MADV_NOHUGEPAGE.
> * This then, in turn, includes linux/huge_mm.h.
> * linux/huge_mm.h declares hugepage_madvise(), which then tries to
>   reference MADV_NOHUGEPAGE, and the build fails.
>
> This can be reached in other ways too.
>
> So we work around this by including uapi/asm/mman.h instead, which allows
> us to keep hugepage_madvise() inline.
>
> The second issue is that the s390 arch declares PROT_NONE as a value in the
> enum prot_type enumeration.
>
> By updating the include in linux/huge_mm.h, we pull in the PROT_NONE
> declaration (unavoidably, this is ultimately in
> uapi/asm-generic/mman-common.h alongside MADV_NOHUGEPAGE), which collides
> with the enumeration value.
>
> To resolve this, we rename PROT_NONE to PROT_TYPE_DUMMY.
>
> The ordering of these patches is critical, the s390 patch must be applied
> prior to the MADV_NOHUGEPAGE patch, and therefore the two patches are sent
> as a series.
>
> v1:
> * Place patches in series.
> * Correct typo in comment as per James.
>
> previous patches:
> huge_mm.h patch - https://lore.kernel.org/all/20250508-madvise-nohugepage-noop-without-thp-v1-1-e7ceffb197f3@kuka.com/
> s390 patch - https://lore.kernel.org/all/20250514163530.119582-1-lorenzo.stoakes@oracle.com/
>
> Ignacio Moreno Gonzalez (1):
>   mm: madvise: make MADV_NOHUGEPAGE a no-op if !THP
>
> Lorenzo Stoakes (1):
>   KVM: s390: rename PROT_NONE to PROT_TYPE_DUMMY
>
>  arch/s390/kvm/gaccess.c | 8 ++++----
>  include/linux/huge_mm.h | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> --
> 2.49.0

On 5/19/25 7:43 AM, Lorenzo Stoakes wrote:
> Andrew -
>
> OK, I realise there's an issue here with patch 2/2. We're not accounting
> for the fact that madvise() will reject this _anyway_ because
> madvise_behavior_valid() will reject it.

Good catch. The purpose of this patch is to make MADV_NOHUGEPAGE a 
no-op, so we can just simply bail out early? The point of madvise 
behavior check is to avoid taking mmap_lock and walking vmas for invalid 
behavior, but it doesn't consider no-op (I treat op-op as valid but do 
nothing), so if we know this advise is a no-op, we just bail out by 
returning 0.

Maybe MADV_UNMERGEABLE should be no-op too, it returns 0 for !KSM anyway.

Thanks,
Yang


>
> I've tried to be especially helpful here to aid Ignacio in his early
> contributions, but I think it's best now (if you don't mind Igancio) for me
> to figure out a better solution after the merge window.
>
> We're late in the cycle now so I will just resend the 1st patch (for s390)
> separately if you're happy to take that for 6.16? It's a simple rename of
> an entirely static identifier so should present no risk, and is approved by
> the arch maintainers who have also agreed for it to come through the mm
> tree.
>
> Apologies for the mess!
>
> Cheers, Lorenzo
>
> On Thu, May 15, 2025 at 09:15:44PM +0100, Lorenzo Stoakes wrote:
>> Andrew -
>>
>> I hope the explanation below resolves your query about the header include
>> (in [0]), let me know if doing this as a series like this works (we need to
>> enforce the ordering here).
>>
>> Thanks!
>>
>> [0]: 20250514153648.598bb031a2e498b1ac505b60@linux-foundation.org
>>
>>
>>
>> Currently, when somebody attempts to set MADV_NOHUGEPAGE on a system that
>> does not enable CONFIG_TRANSPARENT_HUGEPAGE the confguration option, this
>> results in an -EINVAL error arising.
>>
>> This doesn't really make sense, as to do so is essentially a no-op.
>>
>> Additionally, the semantics of setting VM_[NO]HUGEPAGE in any case are such
>> that, should the attribute not apply, nothing will be done.
>>
>> It therefore makes sense to simply make this operation a noop.
>>
>> However, a fly in the ointment is that, in order to do so, we must check
>> against the MADV_NOHUGEPAGE constant. In doing so, we encounter two rather
>> annoying issues.
>>
>> The first is that the usual include we would import to get hold of
>> MADV_NOHUGEPAGE, linux/mman.h, results in a circular dependency:
>>
>> * If something includes linux/mman.h, we in turn include linux/mm.h prior
>>    to declaring MADV_NOHUGEPAGE.
>> * This then, in turn, includes linux/huge_mm.h.
>> * linux/huge_mm.h declares hugepage_madvise(), which then tries to
>>    reference MADV_NOHUGEPAGE, and the build fails.
>>
>> This can be reached in other ways too.
>>
>> So we work around this by including uapi/asm/mman.h instead, which allows
>> us to keep hugepage_madvise() inline.
>>
>> The second issue is that the s390 arch declares PROT_NONE as a value in the
>> enum prot_type enumeration.
>>
>> By updating the include in linux/huge_mm.h, we pull in the PROT_NONE
>> declaration (unavoidably, this is ultimately in
>> uapi/asm-generic/mman-common.h alongside MADV_NOHUGEPAGE), which collides
>> with the enumeration value.
>>
>> To resolve this, we rename PROT_NONE to PROT_TYPE_DUMMY.
>>
>> The ordering of these patches is critical, the s390 patch must be applied
>> prior to the MADV_NOHUGEPAGE patch, and therefore the two patches are sent
>> as a series.
>>
>> v1:
>> * Place patches in series.
>> * Correct typo in comment as per James.
>>
>> previous patches:
>> huge_mm.h patch - https://lore.kernel.org/all/20250508-madvise-nohugepage-noop-without-thp-v1-1-e7ceffb197f3@kuka.com/
>> s390 patch - https://lore.kernel.org/all/20250514163530.119582-1-lorenzo.stoakes@oracle.com/
>>
>> Ignacio Moreno Gonzalez (1):
>>    mm: madvise: make MADV_NOHUGEPAGE a no-op if !THP
>>
>> Lorenzo Stoakes (1):
>>    KVM: s390: rename PROT_NONE to PROT_TYPE_DUMMY
>>
>>   arch/s390/kvm/gaccess.c | 8 ++++----
>>   include/linux/huge_mm.h | 5 +++++
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> --
>> 2.49.0

On Mon, May 19, 2025 at 12:31:04PM -0700, Yang Shi wrote:
>
>
> On 5/19/25 7:43 AM, Lorenzo Stoakes wrote:
> > Andrew -
> >
> > OK, I realise there's an issue here with patch 2/2. We're not accounting
> > for the fact that madvise() will reject this _anyway_ because
> > madvise_behavior_valid() will reject it.
>
> Good catch. The purpose of this patch is to make MADV_NOHUGEPAGE a no-op, so
> we can just simply bail out early? The point of madvise behavior check is to
> avoid taking mmap_lock and walking vmas for invalid behavior, but it doesn't
> consider no-op (I treat op-op as valid but do nothing), so if we know this
> advise is a no-op, we just bail out by returning 0.

Yeah I was thinking better to just do it in mm/madvise.c to be honest.

>
> Maybe MADV_UNMERGEABLE should be no-op too, it returns 0 for !KSM anyway.

Ack, yes could maybe bundle up?

Anyway I didn't want to delay the s390 fix any longer and this is a mess now so
better to defer to next cycle I think :>)

>
> Thanks,
> Yang
>
>
> >
> > I've tried to be especially helpful here to aid Ignacio in his early
> > contributions, but I think it's best now (if you don't mind Igancio) for me
> > to figure out a better solution after the merge window.
> >
> > We're late in the cycle now so I will just resend the 1st patch (for s390)
> > separately if you're happy to take that for 6.16? It's a simple rename of
> > an entirely static identifier so should present no risk, and is approved by
> > the arch maintainers who have also agreed for it to come through the mm
> > tree.
> >
> > Apologies for the mess!
> >
> > Cheers, Lorenzo
> >
> > On Thu, May 15, 2025 at 09:15:44PM +0100, Lorenzo Stoakes wrote:
> > > Andrew -
> > >
> > > I hope the explanation below resolves your query about the header include
> > > (in [0]), let me know if doing this as a series like this works (we need to
> > > enforce the ordering here).
> > >
> > > Thanks!
> > >
> > > [0]: 20250514153648.598bb031a2e498b1ac505b60@linux-foundation.org
> > >
> > >
> > >
> > > Currently, when somebody attempts to set MADV_NOHUGEPAGE on a system that
> > > does not enable CONFIG_TRANSPARENT_HUGEPAGE the confguration option, this
> > > results in an -EINVAL error arising.
> > >
> > > This doesn't really make sense, as to do so is essentially a no-op.
> > >
> > > Additionally, the semantics of setting VM_[NO]HUGEPAGE in any case are such
> > > that, should the attribute not apply, nothing will be done.
> > >
> > > It therefore makes sense to simply make this operation a noop.
> > >
> > > However, a fly in the ointment is that, in order to do so, we must check
> > > against the MADV_NOHUGEPAGE constant. In doing so, we encounter two rather
> > > annoying issues.
> > >
> > > The first is that the usual include we would import to get hold of
> > > MADV_NOHUGEPAGE, linux/mman.h, results in a circular dependency:
> > >
> > > * If something includes linux/mman.h, we in turn include linux/mm.h prior
> > >    to declaring MADV_NOHUGEPAGE.
> > > * This then, in turn, includes linux/huge_mm.h.
> > > * linux/huge_mm.h declares hugepage_madvise(), which then tries to
> > >    reference MADV_NOHUGEPAGE, and the build fails.
> > >
> > > This can be reached in other ways too.
> > >
> > > So we work around this by including uapi/asm/mman.h instead, which allows
> > > us to keep hugepage_madvise() inline.
> > >
> > > The second issue is that the s390 arch declares PROT_NONE as a value in the
> > > enum prot_type enumeration.
> > >
> > > By updating the include in linux/huge_mm.h, we pull in the PROT_NONE
> > > declaration (unavoidably, this is ultimately in
> > > uapi/asm-generic/mman-common.h alongside MADV_NOHUGEPAGE), which collides
> > > with the enumeration value.
> > >
> > > To resolve this, we rename PROT_NONE to PROT_TYPE_DUMMY.
> > >
> > > The ordering of these patches is critical, the s390 patch must be applied
> > > prior to the MADV_NOHUGEPAGE patch, and therefore the two patches are sent
> > > as a series.
> > >
> > > v1:
> > > * Place patches in series.
> > > * Correct typo in comment as per James.
> > >
> > > previous patches:
> > > huge_mm.h patch - https://lore.kernel.org/all/20250508-madvise-nohugepage-noop-without-thp-v1-1-e7ceffb197f3@kuka.com/
> > > s390 patch - https://lore.kernel.org/all/20250514163530.119582-1-lorenzo.stoakes@oracle.com/
> > >
> > > Ignacio Moreno Gonzalez (1):
> > >    mm: madvise: make MADV_NOHUGEPAGE a no-op if !THP
> > >
> > > Lorenzo Stoakes (1):
> > >    KVM: s390: rename PROT_NONE to PROT_TYPE_DUMMY
> > >
> > >   arch/s390/kvm/gaccess.c | 8 ++++----
> > >   include/linux/huge_mm.h | 5 +++++
> > >   2 files changed, 9 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.49.0
>
>