Message-ID: <50e194c2-914d-43eb-bff8-47c4a1119dce@redhat.com>
Date: Tue, 10 Dec 2024 17:59:08 +0100
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Jann Horn <jannh@...gle.com>, Vlastimil Babka <vbabka@...e.cz>,
 Andrew Morton <akpm@...ux-foundation.org>,
 "Liam R . Howlett" <Liam.Howlett@...cle.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] MAINTAINERS: group all VMA-related files into the VMA
 section

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

Powered by blists - more mailing lists