On 2/22/24 22:19, Vlastimil Babka wrote:
> On 2/22/24 20:27, Liam R. Howlett wrote:
>> * Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [240222 13:56]:
>>> * Vlastimil Babka <vbabka@xxxxxxx> [240222 11:56]:
>>> This separates the check for potentially merging previous to a later
>>> failure case. Would it be better to check:
>>> if (curr && curr->vm_ops && curr->vm_ops->close)
>>>
>>> and not set merge_prev = true, ie we cannot merge with the predecessor?
>
> Good suggestion, thanks!
Or actually as Lorenzo informed me, it would affect case 5 as well and we
don't want that. And special casing 5 vs 7 that early would be ugly again.
So I'll just do the code dedup Lorenzo suggested...
>>> That way we would exit as merge_prev == false.
>>>
>>> We would have the added benefit of not having to look at merge_prev &
>>> merge_next case with this vm_ops->close in mind (case 1 and 6).. because
>>> I'm pretty sure we can currently get to case 6 in this way:
>>>
>>> merge_prev = true
>>> check for merge_next.. can_vma_merge_before(next...);
>>> is_mergeable_vma(next.... , true);
>>> if (true && next->vm_ops && next->vm_ops->close) /* Fine for next.. */
>>>
>>> Remove curr by case 6 without checking curr->vm_ops &&
>>> curr->vm_ops->close
>>>
>>> If I am correct, then are we blaming the right commit?
>
> It was bisected with no nondeterminism in the test, so yeah.
>
>> I am not correct.
>> The file check will ensure the same ops, so the file and ops must match.
>> As long as both are checked on one VMA then it will work as required.
>
> Right, otherwise we would have bigger issues even before the buggy commit,
> we were never checking curr's vma_ops before.
>
>>>
>>> Perhaps we should just fail earlier when we find a curr with the close
>>> ops?
>>
>> I'd rather fail earlier, but it's not a big deal.
>
> Your suggestion will indeed result in a nicer and more obvious code, so will
> do, thanks!
>
>>>
>>> > } else { /* case 5 */
>>> > + err = dup_anon_vma(prev, curr, &anon_dup);
>>> > adjust = curr;
>>> > adj_start = (end - curr->vm_start);
>>> > }
>>> > --
>>> > 2.43.1
>>> >
>
