Re: [PATCH V3] x86 spinlock: Fix memory corruption on completing completions

[Raghavendra K T]

On 02/12/2015 08:30 PM, Peter Zijlstra wrote:
> On Thu, Feb 12, 2015 at 05:17:27PM +0530, Raghavendra K T wrote:
[...]
> > Linus suggested that we should not do any writes to lock after unlock(),
> > and we can move slowpath clearing to fastpath lock.
> >
> > So this patch implements the fix with:
> > 1. Moving slowpath flag to head (Oleg).
> > 2. use xadd to avoid read/write after unlock that checks the need for
> > unlock_kick (Linus).
>
> Maybe spend a few more words explaining these things; something like:
>
> Unlocked locks don't care about the slowpath flag; therefore we can keep
> it set after the last unlock, as long as we clear it again on the first
> (try)lock -- this removes the write after unlock.

Nit: I 'll reword a bit here since slowpath flag would result in
unnecessary kick but otherwise harmless IMO.
something like:
Unlocked locks don't care about the slowpath flag; therefore we can keep
it set after the last unlock, and clear it again on the first (try)lock.
-- this removes the write after unlock. note that keeping slowpath flag 
would result in unnecessary kicks.

> By moving the slowpath flag from the tail to the head ticket we avoid
> the need to access both the head and tail tickets on unlock.
>
> We can further avoid the need for a read-after-release by using xadd;
> the prev head value will include the slowpath flag and indicate if we
> need to do PV kicking of suspended spinners -- on modern chips xadd
> isn't (much) more expensive than an add + load.
>
> Its 'obvious' now, but maybe not so much after we've all not looked at
> this for a few months.

Absolutely correct. Thanks Peter for the detailed and very helpful
writeup.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/