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

David Hildenbrand
2024-12-09T14:57:11Z

On 09.12.24 15:28, Lorenzo Stoakes wrote:
> On Mon, Dec 09, 2024 at 03:09:26PM +0100, David Hildenbrand wrote:
>> On 09.12.24 11:06, Lorenzo Stoakes wrote:
>>> On Mon, Dec 09, 2024 at 10:16:21AM +0100, David Hildenbrand wrote:
>>>> On 06.12.24 20:16, Lorenzo Stoakes wrote:
>>>>> There are a number of means of interacting with VMA operations within mm,
>>>>> and we have on occasion not been made aware of impactful changes due to
>>>>> these sitting in different files, most recently in [0].
>>>>>
>>>>> Correct this by bringing all VMA operations under the same section in
>>>>> MAINTAINERS. Additionally take the opportunity to combine MEMORY MAPPING
>>>>> with VMA as there needn't be two entries as they amount to the same thing.
>>>>>
>>>>> [0]:https://lore.kernel.org/linux-mm/CAG48ez0siYGB8GP5+Szgj2ovBZAkL6Zi4n6GUAjzzjFV9LTkRQ@mail.gmail.com/
>>>>>
>>>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>>>> ---
>>>>> MAINTAINERS | 19 +++++++------------
>>>>> 1 file changed, 7 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 1e930c7a58b1..95db20c26f5f 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -15060,18 +15060,6 @@ F: tools/mm/
>>>>> F: tools/testing/selftests/mm/
>>>>> N: include/linux/page[-_]*
>>>>>
>>>>> -MEMORY MAPPING
>>>>> -M: Andrew Morton <akpm@linux-foundation.org>
>>>>> -M: Liam R. Howlett <Liam.Howlett@oracle.com>
>>>>> -M: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>>>> -R: Vlastimil Babka <vbabka@suse.cz>
>>>>> -R: Jann Horn <jannh@google.com>
>>>>> -L: linux-mm@kvack.org
>>>>> -S: Maintained
>>>>> -W: http://www.linux-mm.org
>>>>> -T: git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
>>>>> -F: mm/mmap.c
>>>>> -
>>>>> MEMORY TECHNOLOGY DEVICES (MTD)
>>>>> M: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> M: Richard Weinberger <richard@nod.at>
>>>>> @@ -25028,6 +25016,13 @@ L: linux-mm@kvack.org
>>>>> S: Maintained
>>>>> W: https://www.linux-mm.org
>>>>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
>>>>> +F: mm/madvise.c
>>>>> +F: mm/mlock.c
>>>>> +F: mm/mmap.c
>>>>> +F: mm/mprotect.c
>>>>> +F: mm/mremap.c
>>>>> +F: mm/mseal.c
>>>>> +F: mm/msync.c
>>>>
>>>> Not sure about mprotect.c, mlock.c and madvise.c, though. I'd claim that the
>>>> real "magic" they perform is in page table handling and not primarily VMA
>>>> handling (yes, both do VMA changes, but they are the "easy" part ;) ).
>>>
>>> And large parts of the VMA logic interface with page tables, see - the entire
>>> 6.12 cycle - around recent changes in mmap() MAP_FIXED - which... the VMA
>>> maintainers fixed :)
>>>
>>> And then there were the issues around VMA and mm locking relating to page
>>> table work which... oh right yeah we had to fix again... :>)
>>>
>>> I mean you can make this argument about probably all of these files (mremap
>>> has -tons- of page table-specific stuff), and then we get back to not being
>>> notified about key changes that interface with memory mapping/VMA which we
>>> end up having to deal with anyway.
>>>
>>> A lot of the reason we have 'magic' in these files anyway is because we
>>> don't have a decent generic page table handler. Not sure I'd actually use
>>> the word 'magic' for that though.
>>>
>>> I am planning to make significant changes to mprotect/mlock soon, which
>>> have some terribly duplicated horrible handling logic, and both are key
>>> considerations in VMA logic as a whole.
>>>
>>> Anyway, as far as I'm concerned page table manipulation after the point of
>>> faulting is completely within the purvue of VMA manipulation and a side
>>> product of it.
>>>
>>> However, can concede mm/madvise.c if you feel strongly about that as that's
>>> a bit blurry, but of course contains a whole bunch of VMA and... page table
>>> manipulation :) I mean it still to me seems very pertinent.
>>>
>>
>> And then we have mprotect.c being heavily used by uffd-wp and NUMA hinting,
>> which don't perform any VMA modification.
>>
>> That's why I don't think the change proposed here is really the right step.
>>
>>>>>> They have much more in common with memory.c, which I wouldn't want to
>> see in
>>>> here either. Hm.
>>>
>>> No, memory.c is really dedicated to fault handling. This is really
>>> different from manipulating page tables in specific cases in my opinion.
>>
>> And fork and such stuff. And if you look into huge_memory.c, we actually
>> moved all of the THP logic for mprotect()/madvise()/... in there.
>>
>> Not sure if something similar should have been done for memory.c, or if the
>> THP stuff should actually also have gone into the respective files.
>>
>> To me it sounds wrong to have VMA maintainers maintain a lot of the code in
>> these files code because these files somehow modify VMAs, sorry.
>
> This isn't what I said, I said that de facto we (that is the MEMORY MAPPING
> maintainers as well as VMA) were dealing with a great many issues around
> page tables and page table manipulation which are rather inseparable from
> one another.
>
> I even went to the lengths of writing a detailed set of documentation on
> locking behaviour in and around page table manipulation and solved
> security-sensitive issues in relation to page table teardown over the 6.12
> rc cycle.
>
> To me, the idea that mprotect() and mlock(), operations that are explicitly
> about manipulating VMAs (_and of course consqeuent page table
> manipulation_), are somehow separate is really bizarre to me, but I respect
> your opinion even if I disagree.
> > But unfortunately your arguments apply equally as well to mremap.c
(more
> than half of which is dedicated to page table manipulation), so I will have
> to drop the whole patch then.
>
> If issues arise there in future, I guess others will have to deal with them
> if we don't notice them (luckily Jann did and pinged this time, hopefully
> will in future).

Again, the main point I have is that basic VMA handling (the now very
nice vma.c! ) that wouldn't even require page table modifications (the
very nice testing framework you added) is different to users of that
functionality.

And I agree that many VMA modification users imply page table
modifications and more.

>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
>
> To be clear, I made this change in the interests of the community and
> contributing. It seems to me that within mm has far too little sharing of
> the maintainership burden and I only wanted to help with that and make
> explicit what I work on day-to-day.

And I appreciate that. But putting everything that touches VMAs under
VMA is wrong to me.

>
> I am glad you at least don't object to my doing so with respect to at least
> some parts of the VMA logic.

I really enjoy how well you separated the core VMA logic from everything
else.

--
Cheers,

David / dhildenb