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

[David Hildenbrand <david@redhat.com>]

On 09.12.24 15:45, Lorenzo Stoakes wrote:
> On Mon, Dec 09, 2024 at 03:38:28PM +0100, Jann Horn wrote:
>> On Mon, Dec 9, 2024 at 3:11 PM Lorenzo Stoakes
>> <lorenzo.stoakes@oracle.com> wrote:
>>> On Mon, Dec 09, 2024 at 03:00:08PM +0100, David Hildenbrand wrote:
>>>> On 09.12.24 14:25, Vlastimil Babka wrote:
>>>>> On 12/9/24 10:16, 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 ;) ).
>>>>>
>>>>> I'd think that moving vma files into MEMORY MAPPING (and not the other way)
>>>>> would result in a better overal name, that would be a better fit for the
>>>>> newly added files too?
>>>>
>>>> Maybe. I think vma.c should likely have a different set of maintainers than
>>>> madvise.c and mprotect.c. (again, the magic is in page table modifications)
>>>
>>> The bulk of the logic in mremap.c is related to page tables so by this
>>> logic then, that is out too, right?
>>
>> FWIW, I think technically you can have multiple entries in MAINTAINERS
>> that cover the same file, maybe that would make sense for files that
>> belong to multiple parts of the kernel? Or maybe I'm making things too
>> complicated and it'd be simpler to have some kind of more generic
>> "core MM for userspace mappings" entry or such.
> 
> I think it's faintly ludicrous to separate things on the basis of whether
> they explicitly manipulate one part of the kernel or another, and it'd be
> an odd thing to be trusted with one 'portion' of a file based on some fuzzy
> sense of which bits are 'magic' and therefore out of bounds and which are
> presumably not...
> 
> I don't think it makes sense to separate the 'VMA' bits from these files
> other than perhaps refactoring things a bit (badly needed actually).
> 
> The page table manipulation very sorely needs improvement and
> de-duplication, I am somewhat taken aback that it is thought that I might
> not be able to do so given I had already paid serious consideration to
> doing work in this area based on guard page work (not sure if that work
> would now be welcome?)
> 
> To me I politely disagree with the assessment made here, but if a senior
> member of the kernel objects of course I'll withdraw it.
> 
> But yeah I don't think that's workable. We will just have to hope that we
> notice mremap changes that might be problematic going forward, I might
> therefore update my lei settings accordingly...
> 

I have the feeling you take this personally; please don't.

Maybe my other mail with "VMA users" vs. "basic VMA functionality" makes 
it clearer what I mean.

For example, mm/userfaultfd.c also performs VMA modifications. 
kernel/fork.c does a bunch of that. I neither think these should go 
under VMA nor that it should be split up.

Maybe there is a better way to split up things or rework the code; 
likely we'd want this code that works on VMAs to have a clean interface 
with the core vma logic in vma.c, if the current way of handling it is a 
problem for you.

-- 
Cheers,

David / dhildenb