Re: [PATCH] Antigcc bitfield bikeshed

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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@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!).

Jesse

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx