Re: bit fields && data tearing

[Peter Hurley <peter@hurleysoftware.com>]

On 09/04/2014 05:09 AM, Jakub Jelinek wrote:
> On Thu, Sep 04, 2014 at 10:57:40AM +0200, Mikael Pettersson wrote:
>> Benjamin Herrenschmidt writes:
>>  > On Wed, 2014-09-03 at 18:51 -0400, Peter Hurley wrote:
>>  > 
>>  > > Apologies for hijacking this thread but I need to extend this discussion
>>  > > somewhat regarding what a compiler might do with adjacent fields in a structure.
>>  > > 
>>  > > The tty subsystem defines a large aggregate structure, struct tty_struct.
>>  > > Importantly, several different locks apply to different fields within that
>>  > > structure; ie., a specific spinlock will be claimed before updating or accessing
>>  > > certain fields while a different spinlock will be claimed before updating or
>>  > > accessing certain _adjacent_ fields.
>>  > > 
>>  > > What is necessary and sufficient to prevent accidental false-sharing?
>>  > > The patch below was flagged as insufficient on ia64, and possibly ARM.
>>  > 
>>  > We expect native aligned scalar types to be accessed atomically (the
>>  > read/modify/write of a larger quantity that gcc does on some bitfield
>>  > cases has been flagged as a gcc bug, but shouldn't happen on normal
>>  > scalar types).
>>  > 
>>  > I am not 100% certain of "bool" here, I assume it's treated as a normal
>>  > scalar and thus atomic but if unsure, you can always use int.
>>
>> Please use an aligned int or long.  Some machines cannot do atomic
>> accesses on sub-int/long quantities, so 'bool' may cause unexpected
>> rmw cycles on adjacent fields.
> 
> Yeah, at least pre-EV56 Alpha performs rmw cycles on char/short accesses
> and thus those are not atomic.

Ok, thanks.

And I just confirmed with the Alpha cross-compiler that the fields are
not 'padded out' if volatile either.

Do any arches consider this an 'optimization'?  I ask because this kind of
accidental adjacency sharing may be common. Even RCU has a char field,
rcu_read_unlock_special, in the middle of the task_struct; luckily the
adjacent field is a list_head.

Besides updating the documentation, it may make sense to do something
arch-specific. Just bumping out storage on arches that don't need it
seems wasteful, as does generating bus locks on arches that don't need it.
Unfortunately, the code churn looks unavoidable.

Regards,
Peter Hurley