Re: [RFC PATCH v2 01/10] mm/mremap: introduce more mergeable mremap via MREMAP_RELOCATE_ANON

[Wei Yang <richard.weiyang@gmail.com>]

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