Message-ID: <e1ff054e-bfaf-48d1-9d6f-46ea73d09ac9@lucifer.local>
Date: Wed, 21 May 2025 15:57:17 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Ryan Roberts <ryan.roberts@....com>
Cc: Alexandre Ghiti <alexghiti@...osinc.com>, Will Deacon <will@...nel.org>,
        Alexandre Ghiti <alex@...ti.fr>,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-mm@...ck.org
Subject: Re: [PATCH v5 0/9] Merge arm64/riscv hugetlbfs contpte support

-cc my gmail, I no longer check kernel mail here at all, everything is via my
 work mail (lorenzo.stoakes@...cle.com :)

So apologies for missing this.

On Fri, May 09, 2025 at 02:02:03PM +0100, Ryan Roberts wrote:
> On 09/05/2025 12:09, Alexandre Ghiti wrote:
> > Hi Will,
> >
> > On Thu, May 8, 2025 at 2:30 PM Will Deacon <will@...nel.org> wrote:
> >>
> >> Hi folks,
> >>
> >> On Mon, May 05, 2025 at 06:08:50PM +0200, Alexandre Ghiti wrote:
> >>> On 29/04/2025 16:09, Ryan Roberts wrote:
> >>>> On 07/04/2025 13:04, Alexandre Ghiti wrote:
> >>>>> Can someone from arm64 review this? I think it's preferable to share the same
> >>>>> implementation between riscv and arm64.
> >>>> I've been thinking about this for a while and had some conversations internally.
> >>>> This patchset has both pros and cons.
> >>>>
> >>>> In the pros column, it increases code reuse in an area that has had quite of few
> >>>> bugs popping up lately; so this would bring more eyes and hopefully higher
> >>>> quality in the long run.
> >>>>
> >>>> But in the cons column, we have seen HW errata in similar areas in the past and
> >>>> I'm nervous that by hoisting this code to mm, we make it harder to workaround
> >>>> any future errata. Additionally I can imagine that this change could make it
> >>>> harder to support future Arm architecture enhancements.
> >>>>
> >>>> I appreciate the cons are not strong *technical* arguments but nevertheless they
> >>>> are winning out in this case; My opinion is that we should keep the arm64
> >>>> implementations of huge_pte_ (and contpte_ too - I know you have a separate
> >>>> series for this) private to arm64.
> >>>>
> >>>> Sorry about that.
> >>>>
> >>>>> The end goal is the support of mTHP using svnapot on riscv, which we want soon,
> >>>>> so if that patchset does not gain any traction, I'll just copy/paste the arm64
> >>>>> implementation into riscv.
> >>>> This copy/paste approach would be my preference.
> >>>
> >>>
> >>> I have to admit that I disagree with this approach, the riscv and arm64
> >>> implementations are *exactly* the same so it sounds weird to duplicate code,
> >>> the pros you mention outweigh the cons.
> >>>
> >>> Unless I'm missing something about the erratas? To me, that's easily fixed
> >>> by providing arch specific overrides no? Can you describe what sort of
> >>> erratas would not fit then?
>
> One concrete feature is the use of Arm's FEAT_BBM level 2 to avoid having to do
> break-before-make and TLB maintenance when doing a fold or unfold operation.
> There is a series in flight to add this support at [1]. I can see this type of
> approach being extended to the hugetlb helpers in future.
>
> I also have another series in flight at [2] that tidies up the hugetlb
> implementation and does some optimizations. But the optimizations depend on
> arm64-specific TLB maintenance APIs.
>
> [1]
> https://lore.kernel.org/linux-arm-kernel/20250428153514.55772-2-miko.lenczewski@arm.com/
>
> [2]
> https://lore.kernel.org/linux-arm-kernel/20250422081822.1836315-1-ryan.roberts@arm.com/
>
> As for errata, that's obviously much more fuzzy; there have been a bunch
> relating to the MMU in the recent past, and I wouldn't be shocked if more turned up.
>
> For future architecture enchancements, I'm aware of one potential feature being
> discussed for which this change would likely make it harder to implement.
>
> >>
> >> If we start with the common implementation you have here, nothing
> >> prevents us from forking the code in future if the architectures diverge
> >> so I'd be inclined to merge this series and see how we get on.
>
> OK if that's your preference, I'm ok with it. I don't have strong opinion, just
> a sense that we will end up with loads of arch-specific overrides. As you say,
> let's see.
>
> Alexandre, I guess this series is quite old now and will need to incorporate the
> hugtelb fixes I did last cycle? And ideally I'd like [2] to land then for that
> to also be incorporated into your next version. (I'm still hopeful we can get
> [2] into v6.16 and have been waiting patiently for Will to pick it up ;) ).
>
> I guess we can worry about [1] later as that is only affected by your other series.
>
> How does that sound?
>
> >> However,
> >> one thing I *do* think we need to ensure is that the relevant folks from
> >> both arm64 (i.e. Ryan) and riscv (i.e. Alexandre) are cc'd on changes to
> >> the common code. Otherwise, it's going to be a step backwards in terms
> >> of maintainability.
> >>>> Could we add something to MAINTAINERS so that the new file picks you both
> >> up as reviewers?
>
> That's fine with me. Lorenzo added me for some parts of MM this cycle anyway.
>
> Thanks,
> Ryan

Indeed :) happy to have you there Ryan!

>
> >
> > I'm adding Lorenzo as he is cleaning the mm MAINTAINERS entries.
> >
> > @Lorenzo: should we add a new section "CONTPTE" for this? FYI, hugetlb
> > is the first patchset, I have another patchset to merge THP contpte
> > support [1] as well so the "HUGETLB" section does not seem to be a
> > good fit.

Hm, this does seem to be very arm64-specific right?

But having said that, literally can see risc v entries :)

We are in a strange sort of scenario where there's some cross-over here.

I don't strictly object to it though, this stuff is important and we should get
the mm files absolutely under an appropriate MAINTAINER entry.

So right now it seems the files would consist of:

include/linux/hugetlb_contpte.h
mm/hugetlb_contpte.c

Is this correct?

Is this series intended to be taken by Andrew or through an arch tree?

And who would you sensibly propose for M's and R's?

If we are definitely adding things that sit outside hugetlb or anything
arch-specific, and is in fact generic mm code, then yes this should be a
section.

Does contpte stand for 'Contiguous PTE'?

Then entry could perhaps be:

MEMORY MANAGEMENT - CONTPTE (CONTIGUOUS PTE SUPPORT)

I'd say this entry should probably be added as a patch in this series.

If you give me a list of R's and M's and confirm those files I can very quickly
copy/pasta from an existing entry and then you could respin (and cc my work mail
for the series :P) and include that as an additional patch?

Happy to ACK that in that case.


> >
> > [1] https://lore.kernel.org/linux-riscv/20240508191931.46060-1-alexghiti@rivosinc.com/
> >
> > Thanks,
> >
> > Alex
> >
> >>
> >> Will
>

Cheers, Lorenzo

Powered by blists - more mailing lists