[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250501011845.ktbfgymor4oz5sok@master>
Date: Thu, 1 May 2025 01:18:45 +0000
From: Wei Yang <richard.weiyang@...il.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Wei Yang <richard.weiyang@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Suren Baghdasaryan <surenb@...gle.com>,
Matthew Wilcox <willy@...radead.org>,
David Hildenbrand <david@...hat.com>,
Pedro Falcato <pfalcato@...e.de>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 01/10] mm/mremap: introduce more mergeable mremap
via MREMAP_RELOCATE_ANON
On Wed, Apr 30, 2025 at 05:07:40PM +0100, Lorenzo Stoakes wrote:
>On Wed, Apr 30, 2025 at 03:41:19PM +0000, Wei Yang wrote:
>> On Wed, Apr 30, 2025 at 02:15:24PM +0100, Lorenzo Stoakes wrote:
>> >On Wed, Apr 30, 2025 at 12:47:03AM +0000, Wei Yang wrote:
>> >> On Tue, Apr 22, 2025 at 09:09:20AM +0100, Lorenzo Stoakes wrote:
>> >> [...]
>> >> >+bool vma_had_uncowed_children(struct vm_area_struct *vma)
>> >> >+{
>> >> >+ struct anon_vma *anon_vma = vma ? vma->anon_vma : NULL;
>> >> >+ bool ret;
>> >> >+
>> >> >+ if (!anon_vma)
>> >> >+ return false;
>> >> >+
>> >> >+ /*
>> >> >+ * If we're mmap locked then there's no way for this count to change, as
>> >> >+ * any such change would require this lock not be held.
>> >> >+ */
>> >> >+ if (rwsem_is_locked(&vma->vm_mm->mmap_lock))
>> >> >+ return anon_vma->num_children > 1;
>> >>
>> >> Hi, Lorenzo
>> >>
>> >> May I have a question here?
>> >
>> >Just ask the question.
>> >
>>
>> Thanks.
>>
>> My question is the function is expected to return true, if we have forked a
>> vma from this one, right?
>>
>> IMO there are cases when it has one forked child and anon_vma->num_children == 1,
>> which means folios are not exclusively mapped. But the function would return
>> false.
>>
>> Or maybe I misunderstand the logic here.
>
>I mean, it'd be helpful if you delineated which cases these were?
>
Sorry, I should be more specific.
>Presumably you're thiking of something like:
>
>1. Process 1: VMA A is established. num_children == 1 (self-reference is counted).
>2. Process 2: Process 1 forks, VMA B references A, a->num_children++
>3. Process 3: Process 2 forks, VMA C is established (maybe you think b->num_children++?)
Maybe this is the key point. Will explain below at ***.
>4. Unmap vma B, oops, a->num_children == 1 but it still has C!
>
>But that won't happen, as VMA C will be referencing a->anon_vma, so in reality
>a->anon_vma->num_children == 3, then after unmap == 2.
>
The case here could be handled well, I am thinking a little different one.
Here is the case I am thinking about. If my understanding is wrong, please
correct me.
a VMA A
+-----------+ +-----------+
| | ---> | av| == a
+-----------+ +-----------+
\
\
|\ VMA B
| \ +-----------+
| > | av| == b
| +-----------+
\
\ VMA C
\ +-----------+
> | av| == c
+-----------+
1. Process 1: VMA A is established, num_children == 1
2. Process 2: Process 1 forks, a->num_children++ and b->num_children == 0
3. Process 3: Process 2 forks, b->num_children++ => b->number_children == 1
If vma_had_uncowed_children(VMA B), we would check b->number_children and
return false since it is not greater than 1. But we do have a child process 3.
***
Come back the b->num_children. After re-read your example, I guess this is the
key point. In anon_vma_fork(), we do anon_vma->parent->num_children++. So when
fork VMA C, we increase b->num_children instead of a->num_children.
To verify this, I did a quick test in my test cases in
test_fork_grand_child[1]. I see b->num_children is increased to 1 after C is
forked. Will reply in that thread and hope that would be helpful to
communicate the case.
Well, if I am not correct, feel free to correct me :-)
[1]: http://lkml.kernel.org/r/20250429090639.784-3-richard.weiyang@gmail.com
>References to the originally faulted-in anon_vma is propagated through the
>forks.
>
>anon_vma logic is tricky, one of many reasons I want to (significantly) rework
>it.
>
>Though sadly there is a lot of _essential_ complexity, I do think we can do
>better.
>
--
Wei Yang
Help you, Help me
Powered by blists - more mailing lists