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

David Hildenbrand
2024-12-10T16:59:17Z

On 10.12.24 10:51, Lorenzo Stoakes wrote:
> David,

Hi Lorenzo,

sorry for the late reply.

>
> 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.

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

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.

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

> > 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.

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.

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.

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

--
Cheers,

David / dhildenb