[Intel-gfx] [PATCH] Antigcc bitfield bikeshed

[Daniel Vetter]

On Wed, Jun 24, 2015 at 03:56:15PM -0700, Jesse Barnes wrote:
> On 06/22/2015 11:53 PM, Daniel Vetter wrote:
> > On Mon, Jun 22, 2015 at 02:19:51PM -0700, Jesse Barnes wrote:
> >> On 06/17/2015 08:10 AM, Daniel Vetter wrote:
> >>> On Wed, Jun 17, 2015 at 05:28:20PM +0300, Jani Nikula wrote:
> >>>> On Wed, 17 Jun 2015, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >>>>> Here's an idea I want to float to see if anyone has a better idea.
> >>>>
> >>>> I'll give it some thought, but it pains me that things like this make it
> >>>> harder for source code cross referencers and even grep to find what you
> >>>> you're looking for.
> >>>
> >>> The minimal thing we've tossed around on irc (and we only need minimal
> >>> since there's just a few places that need the raw flags field) is to
> >>> hardcode the offsets and check them at runtime ...
> >>
> >> This one scares me a lot too; is there a thread on the memory ordering
> >> macros somewhere I can look at?  The ordering constraints on x86 are
> >> pretty specific... if we need to annotate things in the code somehow
> >> that could be a plus (generally every *mb() should have a fat comment
> >> explaining the issue), but this seems like overkill at first glance.
> > 
> > This isn't about memory ordering at all but trying to implement
> > ACCESS_ONCE (which is only enforcing that gcc doesn't re-load a value and
> > end up with inconsistent control flow). Unfortunately ACCESS_ONCE doesn't
> > work on bitfield. Code example would be:
> > 
> > if (ACCESS_ONCE(obj->active)) {
> > 	/* complicated slowpath */
> > }
> > 
> > return;
> > 
> > Afaiui without the ACCESS_ONCE gcc might be allowed to re-load obj->active
> > and if we're really unluck it will only partiall execute the slowpath
> > since it decided to reload obj->active and it changed meanwhile. Or some
> > other really ugly thing.
> 
> Ah ok so even more sketchy than regular memory barriers since we're
> talking about the compiler messing around with what gets loaded.
> Fortunately I don't see many usages in i915 (one in __i915_wait_request
> that's undocumented) or core DRM (one in atomic_state_init that's
> undocumented), but I can't immediately tell if they're needed or not.
> 
> In the specific case of bitfields it seems like it would be sufficient
> to mark the local variables as volatile?  Or maybe just use open coded
> compiler barrier() functions instead, with accompanying documentation.
> 
> Documentation/memory-barriers.txt is growing more and more interesting
> over the years (definitely more complicated than when I last added to it!).

Yeah I'm honestly not too concerned about gcc making a mess in the two
cases Chris want's to check something locklessly. It's more for
documentation really so that when you read the code that special lockless
access sticks out. Compiler barrier with a local variable might work, but
the nice thing with ACCESS_ONCE&friends is that they also document exactly
what the thing is you read locklessly.

Wrt comments: I thought the rule for comments on barriers is to make sure
you don't forget to explain where the other side of the barrier is. Which
very often is totally non-obvious. With lockless access we should have
comments in headers already which locks protect which data (big emphasis
on "should"), and the macros make it clear that lockless tricks are being
played. So not sure what to add in comments. What do you have in mind?

Aside: The two users in drm&i915 could all be replaced with READ_ONCE I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch