Re: [PATCH 2/4] mm: perform VMA allocation, freeing, duplication in mm

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Apr 25, 2025 at 11:31:12AM +0100, Lorenzo Stoakes wrote:
> On Fri, Apr 25, 2025 at 06:26:00AM -0400, Liam R. Howlett wrote:
> > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250425 06:09]:
> > > On Thu, Apr 24, 2025 at 06:22:30PM -0700, Suren Baghdasaryan wrote:
> > > > On Thu, Apr 24, 2025 at 2:22 PM David Hildenbrand <david@redhat.com> wrote:
> > > > >
> > > > > On 24.04.25 23:15, Lorenzo Stoakes wrote:
> > > > > > Right now these are performed in kernel/fork.c which is odd and a violation
> > > > > > of separation of concerns, as well as preventing us from integrating this
> > > > > > and related logic into userland VMA testing going forward, and perhaps more
> > > > > > importantly - enabling us to, in a subsequent commit, make VMA
> > > > > > allocation/freeing a purely internal mm operation.
> > > > > >
> > > > > > There is a fly in the ointment - nommu - mmap.c is not compiled if
> > > > > > CONFIG_MMU is not set, and there is no sensible place to put these outside
> > > > > > of that, so we are put in the position of having to duplication some logic
> > > >
> > > > s/to duplication/to duplicate
> > >
> > > Ack will fix!
> > >
> > > >
> > > > > > here.
> > > > > >
> > > > > > This isn't ideal, but since nommu is a niche use-case, already duplicates a
> > > > > > great deal of mmu logic by its nature and we can eliminate code that is not
> > > > > > applicable to nommu, it seems a worthwhile trade-off.
> > > > > >
> > > > > > The intent is to move all this logic to vma.c in a subsequent commit,
> > > > > > rendering VMA allocation, freeing and duplication mm-internal-only and
> > > > > > userland testable.
> > > > >
> > > > > I'm pretty sure you tried it, but what's the big blocker to have patch
> > > > > #3 first, so we can avoid the temporary move of the code to mmap.c ?
> > > >
> > > > Completely agree with David.
> > >
> > > Ack! Yes this was a little bit of a silly one :P
> > >
> > > > I peeked into 4/4 and it seems you want to keep vma.c completely
> > > > CONFIG_MMU-centric. I know we treat NOMMU as an unwanted child but
> > > > IMHO it would be much cleaner to move these functions into vma.c from
> > > > the beginning and have an #ifdef CONFIG_MMU there like this:
> > > >
> > > > mm/vma.c
> > >
> > > This isn't really workable, as the _entire file_ basically contains
> > > CONFIG_MMU-specific stuff. so it'd be one huge #ifdef CONFIG_MMU block with
> > > one small #else block. It'd also be asking for bugs and issues in nommu.
> > >
> > > I think doing it this way fits the patterns we have established for
> > > nommu/mmap separation, and I would say nommu is enough of a niche edge case
> > > for us to really not want to have to go to great lengths to find ways of
> > > sharing code.
> > >
> > > I am quite concerned about us having to consider it and deal with issues
> > > around it so often, so want to try to avoid that as much as we can,
> > > ideally.
> >
> > I think you're asking for more issues the way you have it now.  It could
> > be a very long time until someone sees that nommu isn't working,
> > probably an entire stable kernel cycle.  Basically the longest time it
> > can go before being deemed unnecessary to fix.
> >
> > It could also be worse, it could end up like the arch code with bugs
> > over a decade old not being noticed because it was forked off into
> > another file.
> >
> > Could we create another file for the small section of common code and
> > achieve your goals?
>
> That'd completely defeat the purpose of isolating core functions to vma.c.
>
> Again, I don't believe that bending over backwards to support this niche
> use is appropriate.
>
> And if you're making a change to vm_area_alloc(), vm_area_free(),
> vm_area_init_from(), vm_area_dup() it'd seem like an oversight not to check
> nommu right?
>
> There's already a large amount of duplicated logic there specific to nommu
> for which precisely the same could be said, including entirely parallel
> brk(), mmap() implementations.
>
> So this isn't a change in how we handle nommu.

I guess an alternative is to introduce a new vma_shared.c, vma_shared.h
pair of files here, that we try to allow userland isolation for so vma.c
can still use for userland testing.

This then aligns with your requirement, and keeps it vma-centric like
Suren's suggestion.

Or perhaps it could even be vma_init.c, vma_init.h? To denote that it
references the initialisation and allocation, etc. of VMAs?

Anyway we do that, we share it across all, and it solves all
problems... gives us the isolation for userland testing and also isolation
in mm, while also ensuring no code duplication with nommu.

That work?


>
> >
> > Thanks,
> > Liam
> >
> >