Re: [RFC][PATCH] spinlock: Kill spin_unlock_wait()

[Nick Piggin <npiggin@kernel.dk>]

On Thu, Jan 06, 2011 at 10:32:33AM +0100, Peter Zijlstra wrote:
> On Wed, 2011-01-05 at 11:43 -0800, Linus Torvalds wrote:
> > On Wed, Jan 5, 2011 at 11:14 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > >
> > > There appear to be only two callsites of said horror, one in the exit
> > > path and one in ata-eh, neither appear to be performance critical so I
> > > replaced them with a simple lock-unlock sequence.
> > 
> > Again, WHY?
> > 
> > What's the problem with the current code? Instead of generating ugly
> > patches to change it, and instead of removing it, just say what the
> > PROBLEM is.
> 
> Well, I don't care about the primitive anymore, and Nick had some
> reasonable arguments on why its not a good primitive to have. So in a
> brief moment I decided to see what it would take to make it go away.
> 
> Apparently you don't like it, I'm fine with that, consider the patch
> discarded.
> 
> > Some simple helper functions to extract the tail/head part of the
> > ticket lock to make the comparisons understandable,
> 
> Jeremy has a number of pending patches making things more pretty. If you
> wish I can revisit this once that work hits your tree.
> 
>   http://lkml.org/lkml/2010/11/16/479
> 
> He makes the thing looks like:
> 
> +#if (CONFIG_NR_CPUS < 256)
> +typedef u8  __ticket_t;
> +#else
> +typedef u16 __ticket_t;
> +#endif
> +
> +#define TICKET_SHIFT   (sizeof(__ticket_t) * 8)
> +#define TICKET_MASK    ((__ticket_t)((1 << TICKET_SHIFT) - 1))
> +
>  typedef struct arch_spinlock {
> +       union {
> +               unsigned int slock;
> +               struct __raw_tickets {
> +                       __ticket_t head, tail;
> +               } tickets;
> +       };
>  } arch_spinlock_t;
> 
> >  together with
> > always accessing the lock with the proper ACCESS_ONCE() would have
> > made your previous patch acceptable.
> 
> I'm still not quite seeing where I was missing an ACCESS_ONCE(), the
> second loop had a cpu_relax() in, which is a compiler barrier so it
> forces a reload that way.
> 
> >  But you ignored that feedback,
> > and instead you now want to do a "let's just remove it entirely patch"
> > that is even worse.
> 
> My locking improved and became a lot more obvious by not using the
> primitive, so for the work I was doing not using it seemed the better
> solution.
> 
> And as said, this was inspired by Nick's comments and it was a quick
> edit to see what it would take.

I was hoping more that it could be removed from callers rather than
replaced with lock/unlock sequence...

I can't see how the usage in libata could be right. If we're waiting
for some modifications inside the critical section, then it seems we
could load some shared data before the lock is actually released, on
an architecture which does load/load reordering. At best it needs some
good comments and perhaps a barrier or two.

The spin_unlock_wait in do_exit seems like it shouldn't be needed --
exit_pi_state_list takes the pi_lock after the task has died anyway,
so I can't see what the problem would be. Removing the call (and
perhaps put a BUG_ON(!(curr->flags & PF_EXITING)) in exit_pi_state_list
would do the same thing, avoid the barrier, and localize fuxtex exit
protocol to futex.c.

(If the spin_unlock_wait call *was* actually required, eg. we subsequently
 checked current->pi_state_list without holding the pi_lock, then we
 would require a barrier after the spin_unlock_wait call too, to prevent
 speculative load pulling it up before the load of the lock, so I don't
 really agree Linus that it's an obvious API -- to you perhaps because
 barriers are second nature to you, but not for driver writers :)).