[Intel-gfx] [PATCH 4/4] dma-buf: nuke reservation_object seq number

[Daniel Vetter]

On Wed, Aug 14, 2019 at 06:20:28PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2019-08-14 18:06:26)
> > On Wed, Aug 14, 2019 at 05:42:48PM +0100, Chris Wilson wrote:
> > > Quoting Daniel Vetter (2019-08-14 16:39:08)
> [snip]
> > > > > > >          if (old)
> > > > > > > -               old->shared_count = 0;
> > > > > > > -       write_seqcount_end(&obj->seq);
> > > > > > > +               smp_store_mb(old->shared_count, 0);
> > > > 
> > > > So your comment and the kerneldoc don't match up. Quoting
> > > > Documentation/memory-barriers.txt:
> > > > 
> > > >      This assigns the value to the variable and then inserts a full memory
> > > >      barrier after it.  It isn't guaranteed to insert anything more than a
> > > >      compiler barrier in a UP compilation.
> > > > 
> > > > So order is 1. store 2. fence, but your comment suggests you want it the
> > > > other way round.
> > > 
> > > What's more weird is that it is a fully serialising instruction that is
> > > used to fence first as part of the update. If that's way PeterZ uses
> > > it...
> > 
> > I haven't looked at the implementations tbh, just going with the text. Or
> > do you mean in the write_seqlock that we're replacing?
> 
> Nah, I misremembered set_current_state(), all that implies is the fence
> is before the following instructions. I have some recollection that it
> can be used as a RELEASE operation (if only because it is a locked xchg).
> If all else fails, make it an xchg_release(). Or normal assignment +
> smp_wmb().

Yeah that one is called smp_store_release, not smp_store_mb. I think
smp_store_release is the right one here.

> > > It's an exclusive fence. If it is replaced, it must be later than all
> > > the shared fences (and dependent on them directly or indirectly), and
> > > so still a consistent snapshot.
> > 
> > I'm not worried about the fence, that part is fine. But we're defacto
> > using the fence as a fancy seqlock-of-sorts. And if the fence gets reused
> > and the pointers match, then our seqlock-of-sorts breaks. But I haven't
> > looked around whether there's more in the code that makes this an
> > irrelevant issue.
> 
> No, it should not break if we replace the fence with the same pointer.
> If the fence pointer expires, reused and assigned back as the excl_fence
> -- it is still the excl_fence and by the properties of that
> excl_fence construction, it is later than the shared_fences.

So I thought the rules are that if we have an exclusive fence and shared
fences both present, then the shared fences are after the exclusive one.

But if we race here, then I think we could accidentally sample shared
fences from _before_ the exclusive fences. Rough timeline:

exlusive fence 1 -> shared fence 2 -> exclusive fence, but reuses memory
of fence 1

Then we sample fence 1, capture the shared fence 2, and notice that the
exclusive fence pointer is the same (but not the fence on the timeline)
and conclude that we got a consistent sample.

But the only consistent sample with the new fence state would be only the
exclusive fence.

Reminds me I forgot to look for the usual kref_get_unless_zero trickery we
also need to do here to make sure we have the right fence.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch