Re: [PATCH v2 1/2] mm/rmap: fix and simplify reusing mergeable anon_vma as parent when fork

[Wei Yang]

On Sun, Jan 12, 2020 at 12:55:45PM +0300, Konstantin Khlebnikov wrote:
>
>
>On 12/01/2020 01.38, Wei Yang wrote:
>> On Fri, Jan 10, 2020 at 11:11:23AM +0300, Konstantin Khlebnikov wrote:
>> [...]
>> > > > > > 
>> > > > > > series of vma in parent with shared AV:
>> > > > > > 
>> > > > > > SRC1 - AV0
>> > > > > > SRC2 - AV0
>> > > > > > SRC3 - AV0
>> > > > > > ...
>> > > > > > SRCn - AV0
>> > > > > > 
>> > > > > > in child after fork
>> > > > > > 
>> > > > > > DST1 - AV_OLD_1 (some old vma, picked by anon_vma_clone) plus DST1 is attached to same AVs as SRC1
>> > > > > > DST2 - AV_OLD_2 (other old vma) plus DST1 is attached to same AVs as SRC2
>> > > > > > DST2 - AV1 prev AV parent does not match AV0, no old vma found for reusing -> allocate new one (child of AV0)
>> > > > > > DST3 - AV1 - DST2->AV->parent == SRC3->AV (AV0) -> share AV with prev
>> > > > > > DST4 - AV1 - same thing
>> > > > > > ...
>> > > > > > DSTn - AV1
>> > > > > > 
>> 
>> To focus on the point, I rearranged the order a little. Suppose your following
>> comments is explaining the above behavior.
>> 
>>     I've illustrated how two heuristics (reusing-old and sharing-prev) _could_ work together.
>>     But they both are optional.
>>     At cloning first vma SRC1 -> DST1 there is no prev to share anon vma,
>>     thus works common code which _could_ reuse old vma because it have to.
>>     If there is no old anon-vma which have to be reused then DST1 will allocate
>>     new anon-vma (AV1) and it will be used by DST2 and so on like on your picture.
>> 
>> I agree with your 3rd paragraph, but confused with 2nd.
>> 
>> At cloning first vma SRC1 -> DST1, there is no prev so anon_vma_clone() would
>> pick up a reusable anon_vma. Here you named it AV_OLD_1. This looks good to
>> me. But I am not sure why you would picked up AV_OLD_2 for DST2? In parent,
>> SRC1 and SRC2 has the same anon_vma, AV0. So in child, DST1 and DST2 could
>> also share the same anon_vma, AV_OLD_1.
>> 
>> Sorry for my poor understanding, would you mind giving me more hint on this
>> change?
>
>For DST2 heuristic "share-with-prev" will not work because if prev (DST1)
>uses old AV (AV_OLD_1) and AV_OLD_1->parent isn't SRC2->AV (AV0).
>So DST2 could only pick another old AV or allocate new.

I know this behavior after your change, my question is why you want to do so.

>
>My patch uses condition dst->prev->anon_vma->parent == src->anon_vma rather
>than obvious src->prev->anon_vma == src->anon_vma because in this way it
>eliminates all unwanted corner cases and explicitly verifies that we going to
>share related anon-vma.
>

This do eliminates some corner case, but as you showed child and parent don't
share the same AV topology. To keep the same AV topology is the purpose of my
commit.

I agree you found some bug that previous commit doesn't do it is expected. But
since you change the design a little, I suggest you split this idea to a
separate patch so that reviewer and audience in the future could understand
your approach clearly. Otherwise audience would be confused and hard to track
this change.

For example, you describe the behavior after your change. The second vma would
probably have a different AV from first vma.

>Heuristic "reuse-old" uses fact that VMA links and AV parent chain are tracked
>independently: when VMA reuses old AV it still links to all related AV even
>if VMA->AV points into some old AV in the middle of inheritance chain.
>
>> 
>> > > > > 
>> > > > > Yes, your code works for DST3..DSTn. They will pick up AV1 since
>> > > > > (DST2->AV->parent == SRC3->AV).
>> > > > > 
>> > > > > My question is why DST1 and DST2 has different AV? The purpose of my patch
>> > > > > tries to make child has the same topology and parent. So the ideal look of
>> > > > > child is:
>> > > > > 
>> > > > > DST1 - AV1
>> > > > > DST2 - AV1
>> > > > > DST2 - AV1
>> > > > > DST3 - AV1
>> > > > > DST4 - AV1
>> > > > > 
>> > > > > Would you mind putting more words on DST1 and DST2? I didn't fully understand
>> > > > > the logic here.
>> > > > > 
>> > > > > Thanks
>> > > > > 
>> > > > 
>> > > > I think that the first version is doing the work as you expected, but been
>> > > > revised in second version, to limits the number of users of reused old
>> > > > anon(which is picked in anon_vma_clone() and keep the tree structure.
>> > > > 
>> > > 
>> > > Any reason to reduce the reuse? Maybe I lost some point.
>> > 
>> > > 
>> > > > > --
>> > > > > Wei Yang
>> > > > > Help you, Help me
>> > > 
>> 

-- 
Wei Yang
Help you, Help me