On Wed, 26 Jul 2023 at 14:51, Jann Horn <jannh@xxxxxxxxxx> wrote:
>
> Of course I only realize directly after sending this patch that this
> comment only holds... [..]
> ... if we move the smp_store_release() down by one line here.
So I've applied PATCH 1/2 as obvious, but am holding off on this one.
Partly because of the other discussion about memory ordering, but also
due to how the now sometimes much more complex-looking conditionals
could be made a bit visually simpler.
For example the patch does
- if (!vma || !(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
+ /* see anon_vma_prepare() */
+ if (!vma || !(vma->vm_flags & VM_MERGEABLE) ||
+ !smp_load_acquire(&vma->anon_vma))
return NULL;
which is a understandably mindless "just add the smp_load_acquire()",
but it is also just unnecessarily hard to read.
And the comment placement is a bit misleading too, since it really
only refers to one part of the expression. And that's very obvious if
you know what it's about, but the whole point of that comment would be
that you don't necessarily know why the code is doing what it is
doing.
IOW, that would be much more legible just split up as
if (!vma || !(vma->vm_flags & VM_MERGEABLE))
return NULL;
/* see anon_vma_prepare() */
if (!smp_load_acquire(&vma->anon_vma))
return NULL;
I feel.
Also, I'm now wondering about the ordering of that
> smp_store_release(&vma->anon_vma, anon_vma);
> anon_vma_chain_link(vma, avc, anon_vma);
sequence. Maybe we *do* want the anon_vma pointer to be set first, so
that once it's on that anon_vma chain (and is visible in the anon_vma
interval tree) it always has a proper anon_vma pointer.
*OR* maybe the rule needs to be that any other concurrent user that
sees 'anon_vma' as being valid also can rely on the links being set
up?
I *suspect* the ordering doesn't actually matter, but it just makes me
wonder more about this area.
Linus
