Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi()

[Darren Hart]

On Sun, Dec 20, 2015 at 06:40:07AM +0100, Mike Galbraith wrote:
> On Sat, 2015-12-19 at 21:15 -0800, Darren Hart wrote:
> 
> > As a follow-on, I think it might be worthwhile to create a symmetrical
> > get_pi_state() to the put_pi_state(), rather than handling the atomic_inc
> > directly.
> 
> Ditto, immediate thought was future auditors will look for it.

Hrm, well, I had just dismissed this after some digging, but OK, let's think it
through a bit...

Turns out there is only one open coded atomic_inc. the other occurs through
attach_to_pi_state, sometimes via lookup_pi_state. We might be able to
consolidate that some so it had a more symmetric and consistent usage pattern.

A "get' in the futex op functions currently occurs via:

1) lookup_pi_state -> attach_to_pi_state()

2) attach_to_pi_state()
   (directly - nearly copying lookup_pi_state at the call site)

3) futex_lock_pi_atomic -> attach_to_pi_state()

4) atomic_inc(pi_state->ref_count) in futex_requeue_pi on behalf of the waiter
   in futex_wait_requeue_pi.

Newly allocated or re-purposed pi_states get their refcount set to 1 which
doesn't really count as a "get" until a lookup_pi_state or implicit acquisition
through futex_lock_pi_atomic->attach_to_pi_state.

As it turns out, lookup_pi_state is only used once in futex.c, but it really
does make that section more readable. Despite an overall savings of a couple of
lines, I think it's worth keeping, even if it adds another layer to the "get".

As a further cleanup, Thomas removed the unnecessary calls to put_pi_state, and
those that remain have no ambiguity about whether or not the pi_state is NULL.
We *could* drop the NULL check in put_pi_state, which would make it's use all
the more explicit. Perhaps a BUG_ON(!pi_state)? Not included below.

The first get is still implicit in the way the caching of the pi_state works.
This gets assigned with an existing refcount of 1 in attach_to_pi_owner from the
cache via alloc_pi_state. I don't know if there is a reason for starting it off
as 1 instead of 0. Perhaps we could make this more explicit by keeping it at 0
in the cache and using get_pi_state in alloc_pi_state before giving it to the
waiter?

Something like this perhaps? (Untested):