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.
