Re: [PATCH] MAINTAINERS: group all VMA-related files into the VMA section

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

On Tue, Dec 10, 2024 at 05:59:08PM +0100, David Hildenbrand wrote:
> On 10.12.24 10:51, Lorenzo Stoakes wrote:
> > David,
>
> Hi Lorenzo,
>
> sorry for the late reply.
>

No worries, we're all busy. I'm off on holiday after the end of this week
also so if I disappear after then this is why :)

> >
> > To avoid an infinite back-and-forth...
> >
> > I think you're stuck on the idea that we are sat in a VMA 'vacuum', perhaps
> > because I put so much effort into separating out the VMA logic, for the
> > purpose of being able to userland test it, it's almost given the wrong
> > impression (I should perhaps have put less effort into this? :)
>
> Yes, that nicely summarizes it.
>
> In particular, because the patch description says "group all VMA-related
> files", and I am having a hard time to even identify *what* a "VMA-related"
> file is, really.

Yeah I definitely misgrouped this.

>
> For example, why not mempolicy.c->mbind()? Not that I would suggest that,
> because the file is filled with non-vma-related stuff.

Right, we have a lot of this 'mixing'.

>
> See below of what might make sense to me.
>
> >
> > But we have for quite some time now de facto been expected to maintain all
> > aspects of mapping, remapping, etc. INCLUDING page table considerations.
> > > We are more or less de facto maintaining all that (modulo madvise.c - I
> > grant you this is the odd one out).
> >
> > So you can imagine it's a bit frustrating, when that's the case, to be told
> > in effect - no this isn't for you - right?
>
> It isn't "VMA" group for me, independent of "who" is currently listed there.
> And maybe we now agree on that, maybe not.

Yes absolutely agreed.

>
> Talking about things that are frustrating: being called a "senior member of
> the kernel". :)

Haha well, I still see myself as a raw "junior" if that helps ;) or maybe
makes it worse? I don't know :P

I am at least certainly senior in the sense of wrinkles...

>
> > > For instance, as I said before, we have spent large parts of the 6.12
> cycle
> > dealing with practical concerns specifically around page table
> > manipulation.
> > > Maintainership is more than setting up lei rules, obviously. One can set
> > lei rules for anything. It means that our say has more impact, and it also
> > means that we are (co-)responsible for the listed files, and that's acked
> > rather than disregarded.
> > > So, again, I politely disagree with your assessment re: page tables.
> > > I think their manipulation under circumstances where you
> > map/remap/mprotect/mlock is -inseparable- from memory mapping/VMA
> > logic. Otherwise, what's the point right?
>
> We'll likely have to agree to disagree. :) But again, my main point is that
> the VMA group is misleading.

Yes and civil disagreement is fine! But yes agreed on structure.

>
> As a side note, I believe the code can be structures accordingly (call that
> separating it). mmap/munmap handling is the prime example right now for me.
>
> For example, I really enjoy how:
>
> munmap() (mmap.c) routes to __vm_munmap (vma.c) makes use of abstracted page
> table logic like free_pgtables() and unmap_vmas() (memory.c).
>
> This way it is clear what the rather high-level MM syscall implementation is
> (mmap.c), what the VMA handling is (vma.c) and what the abstracted page
> table handling logic is (memory.c). I don't think that page table handling
> code should be moved to either mmap.c or vma.c.
>
>
> I would enjoy if we would handle e.g., mprotect.c in a similar way, such
> that the helper like change_protection() -- again, used by completely
> mprotect()-unrelated code outside of mprotect.c -- would not reside in
> mprotect.c. But that code just evolved naturally after we kept reusing
> change_protection() outside of the file.

Yeah, so I think another step forward is to start actually moving some of
the more obviously general stuff like this to other files, I will look at
this in the new year.

>
> Regarding mremap logic there once was a discussion about merging it with the
> uffdio_move logic, but not sure if that really makes sense.
>
> >
> > My suggestion is that:
> >
> > a. we put everything in MEMORY MAPPING
> > b. We drop mm/madvise.c from the list
>
> Sounds better, although I am still fuzzy on what is supposed to be in there
> and what not. See my mbind() example above ..
>
> I see how mmap/munmap/mprotect/mremap fall into the same category of MM
> syscalls that mainly affect the /proc/self/maps output (in contrast to a
> bunch of others). Which make sense to me to group in such a way.
>
> mseal.c and mlock.c likely as well.

Yes this is why I think these belong together, and under 'memory mapping'
makes sense if one sees it from this point of view.

>
> msync.c is a simple VMA walker ... not sure if it belongs in there, but who
> am I to tell.

Will drop, was a tenuous one, just seemed like a tiny vaguely VMA-adjacent
thing but agreed it's a bit out of place.

>
> --
> Cheers,
>
> David / dhildenb
>

OK so I think we're (probably?) now agreed, I will submit a patch shortly
that:

a. puts everything in MEMORY MAPPING
b. Drops mm/madvise.c, mm/msync.c from the list
c. I commit to moving things out of the various files that truly belongs
   elsewhere

I mean there's stuff that's weirdly used for page table moving in mremap.c
that should probably live in memory.c as well for instance.