Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1617382 > unrolled thread

Re: [PATCH -v6 05/13] futex: Change locking rules

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH -v6 05/13] futex: Change locking rules Darren Hart <dvhart@infradead.org> - 2017-04-05 23:20 +0200
    Re: [PATCH -v6 05/13] futex: Change locking rules Peter Zijlstra <peterz@infradead.org> - 2017-04-06 14:30 +0200
      Re: [PATCH -v6 05/13] futex: Change locking rules Joe Perches <joe@perches.com> - 2017-04-06 18:00 +0200
      Re: [PATCH -v6 05/13] futex: Change locking rules Darren Hart <dvhart@infradead.org> - 2017-04-06 19:30 +0200

#1617382 — Re: [PATCH -v6 05/13] futex: Change locking rules

On Wed, Mar 22, 2017 at 11:35:52AM +0100, Peter Zijlstra wrote:
> Currently futex-pi relies on hb->lock to serialize everything. Since
> hb->lock is giving us problems (PI inversions among other things,
> since on -rt hb lock itself is a rt_mutex), we want to break this up a
> bit.
> 
> This patch reworks and documents the locking. Notably, it
> consistently uses rt_mutex::wait_lock to serialize {uval, pi_state}.
> This would allow us to do rt_mutex_unlock() (including deboost)
> without holding hb->lock.
> 
> Nothing yet relies on the new locking rules.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/futex.c |  165 +++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 132 insertions(+), 33 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -973,6 +973,39 @@ void exit_pi_state_list(struct task_stru
>   *
>   * [10] There is no transient state which leaves owner and user space
>   *	TID out of sync.
> + *
> + *
> + * Serialization and lifetime rules:
> + *
> + * hb->lock:
> + *
> + *	hb -> futex_q, relation
> + *	futex_q -> pi_state, relation
> + *
> + *	(cannot be raw because hb can contain arbitrary amount
> + *	 of futex_q's)
> + *
> + * pi_mutex->wait_lock:
> + *
> + *	{uval, pi_state}
> + *
> + *	(and pi_mutex 'obviously')
> + *
> + * p->pi_lock:

This documentation uses a mix of types and common variable names. I'd recommend
some declarations just below "Serialization and lifetime rules:" to help make
this explicit, e.g.:

struct futex_pi_state *pi_state;
struct futex_hash_bucket *hb;
struct rt_mutex *pi_mutex;
struct futex_q *q;
task_struct *p;

> + *
> + *	p->pi_state_list -> pi_state->list, relation
> + *
> + * pi_state->refcount:
> + *
> + *	pi_state lifetime
> + *
> + *
> + * Lock order:
> + *
> + *   hb->lock
> + *     pi_mutex->wait_lock
> + *       p->pi_lock
> + *
>   */
>  
>  /*
> @@ -980,10 +1013,12 @@ void exit_pi_state_list(struct task_stru
>   * the pi_state against the user space value. If correct, attach to
>   * it.
>   */
> -static int attach_to_pi_state(u32 uval, struct futex_pi_state *pi_state,
> +static int attach_to_pi_state(u32 __user *uaddr, u32 uval,
> +			      struct futex_pi_state *pi_state,
>  			      struct futex_pi_state **ps)
>  {
>  	pid_t pid = uval & FUTEX_TID_MASK;
> +	int ret, uval2;

The uval should be an unsigned type:

u32 uval2;

>  
>  	/*
>  	 * Userspace might have messed up non-PI and PI futexes [3]
> @@ -991,9 +1026,34 @@ static int attach_to_pi_state(u32 uval,
>  	if (unlikely(!pi_state))
>  		return -EINVAL;
>  
> +	/*
> +	 * We get here with hb->lock held, and having found a
> +	 * futex_top_waiter(). This means that futex_lock_pi() of said futex_q
> +	 * has dropped the hb->lock in between queue_me() and unqueue_me_pi(),

This context got here like this:

	futex_lock_pi
		hb lock
		futex_lock_pi_atomic
			top waiter
			attach_to_pi_state()

	The queue_me and unqueue_me_pi both come after this in futex_lock_pi.
	Also, the hb lock is dropped in queue_me, not between queue_me and
	unqueue_me_pi.

Are you saying that in order to be here, there are at least two tasks contending
for the lock, and one that has come before us has proceeded as far as queue_me()
but has not yet entered unqueue_me_pi(), therefor we know there is a waiter and
it has a pi_state? If so, I think we can make this much clearer by at least
noting the two tasks in play.

...

> @@ -1336,6 +1418,7 @@ static int wake_futex_pi(u32 __user *uad
>  
>  	if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) {
>  		ret = -EFAULT;
> +

Stray whitespace addition? Not explicitly against coding-style, but I don't
normally see a new line before the closing brace leading to an else...

>  	} else if (curval != uval) {
>  		/*
>  		 * If a unconditional UNLOCK_PI operation (user space did not

...

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [next] | [standalone]

#1617941

On Wed, Apr 05, 2017 at 02:18:43PM -0700, Darren Hart wrote:
> On Wed, Mar 22, 2017 at 11:35:52AM +0100, Peter Zijlstra wrote:

> > + *
> > + * Serialization and lifetime rules:
> > + *
> > + * hb->lock:
> > + *
> > + *	hb -> futex_q, relation
> > + *	futex_q -> pi_state, relation
> > + *
> > + *	(cannot be raw because hb can contain arbitrary amount
> > + *	 of futex_q's)
> > + *
> > + * pi_mutex->wait_lock:
> > + *
> > + *	{uval, pi_state}
> > + *
> > + *	(and pi_mutex 'obviously')
> > + *
> > + * p->pi_lock:
> 
> This documentation uses a mix of types and common variable names. I'd recommend
> some declarations just below "Serialization and lifetime rules:" to help make
> this explicit, e.g.:
> 
> struct futex_pi_state *pi_state;
> struct futex_hash_bucket *hb;
> struct rt_mutex *pi_mutex;
> struct futex_q *q;
> task_struct *p;

Yeah, not convinced it helps much. If you're stuck at that level, the
rest of futex is going to make your head explode.

> > @@ -980,10 +1013,12 @@ void exit_pi_state_list(struct task_stru
> >   * the pi_state against the user space value. If correct, attach to
> >   * it.
> >   */
> > +static int attach_to_pi_state(u32 __user *uaddr, u32 uval,
> > +			      struct futex_pi_state *pi_state,
> >  			      struct futex_pi_state **ps)
> >  {
> >  	pid_t pid = uval & FUTEX_TID_MASK;
> > +	int ret, uval2;
> 
> The uval should be an unsigned type:
> 
> u32 uval2;

Right you are.

> >  
> >  	/*
> >  	 * Userspace might have messed up non-PI and PI futexes [3]
> > @@ -991,9 +1026,34 @@ static int attach_to_pi_state(u32 uval,
> >  	if (unlikely(!pi_state))
> >  		return -EINVAL;
> >  
> > +	/*
> > +	 * We get here with hb->lock held, and having found a
> > +	 * futex_top_waiter(). This means that futex_lock_pi() of said futex_q
> > +	 * has dropped the hb->lock in between queue_me() and unqueue_me_pi(),
> 
> This context got here like this:
> 
> 	futex_lock_pi
> 		hb lock
> 		futex_lock_pi_atomic
> 			top waiter
> 			attach_to_pi_state()
> 
> 	The queue_me and unqueue_me_pi both come after this in futex_lock_pi.
> 	Also, the hb lock is dropped in queue_me, not between queue_me and
> 	unqueue_me_pi.
> 
> Are you saying that in order to be here, there are at least two tasks contending
> for the lock, and one that has come before us has proceeded as far as queue_me()
> but has not yet entered unqueue_me_pi(), therefor we know there is a waiter and
> it has a pi_state? If so, I think we can make this much clearer by at least
> noting the two tasks in play.

The point is that this other task must have a reference, and since we
now hold hb->lock, it cannot go away.

> 
> ...
> 
> > @@ -1336,6 +1418,7 @@ static int wake_futex_pi(u32 __user *uad
> >  
> >  	if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) {
> >  		ret = -EFAULT;
> > +
> 
> Stray whitespace addition? Not explicitly against coding-style, but I don't
> normally see a new line before the closing brace leading to an else...

I found it more readable that way. Sod checkpatch and co ;-)

> >  	} else if (curval != uval) {
> >  		/*
> >  		 * If a unconditional UNLOCK_PI operation (user space did not

[toc] | [prev] | [next] | [standalone]

#1618132

On Thu, 2017-04-06 at 14:28 +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2017 at 02:18:43PM -0700, Darren Hart wrote:
> > On Wed, Mar 22, 2017 at 11:35:52AM +0100, Peter Zijlstra wrote:
> > > @@ -1336,6 +1418,7 @@ static int wake_futex_pi(u32 __user *uad
> > >  
> > >  	if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) {
> > >  		ret = -EFAULT;
> > > +
> > 
> > Stray whitespace addition? Not explicitly against coding-style, but I don't
> > normally see a new line before the closing brace leading to an else...
> 
> I found it more readable that way. Sod checkpatch and co ;-)

The only good sod is the stuff you get to play games on.

And this week's best sod is Augusta's immaculate carpet
for the Masters Tournament.

So no worries from me.  checkpatch is a brainless script.
Rules made to be broken, etc.

afaict another way to write that would be to use gotos
and that would be a lot more lines and less readable.

[toc] | [prev] | [next] | [standalone]

#1618213

On Thu, Apr 06, 2017 at 02:28:32PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2017 at 02:18:43PM -0700, Darren Hart wrote:
> > On Wed, Mar 22, 2017 at 11:35:52AM +0100, Peter Zijlstra wrote:
> 
> > > + *
> > > + * Serialization and lifetime rules:
> > > + *
> > > + * hb->lock:
> > > + *
> > > + *	hb -> futex_q, relation
> > > + *	futex_q -> pi_state, relation
> > > + *
> > > + *	(cannot be raw because hb can contain arbitrary amount
> > > + *	 of futex_q's)
> > > + *
> > > + * pi_mutex->wait_lock:
> > > + *
> > > + *	{uval, pi_state}
> > > + *
> > > + *	(and pi_mutex 'obviously')
> > > + *
> > > + * p->pi_lock:
> > 
> > This documentation uses a mix of types and common variable names. I'd recommend
> > some declarations just below "Serialization and lifetime rules:" to help make
> > this explicit, e.g.:
> > 
> > struct futex_pi_state *pi_state;
> > struct futex_hash_bucket *hb;
> > struct rt_mutex *pi_mutex;
> > struct futex_q *q;
> > task_struct *p;
> 
> Yeah, not convinced it helps much. If you're stuck at that level, the
> rest of futex is going to make your head explode.

It just presented one more fork in the mindmap to go confirm types and names so
I was sure I was thinking of the same things as what was documented. Being
explicit avoids unnecessary confusion, reduces thought errors, and takes minimal
effort on our part. Well worth it IMHO.


> 
> > > @@ -980,10 +1013,12 @@ void exit_pi_state_list(struct task_stru
> > >   * the pi_state against the user space value. If correct, attach to
> > >   * it.
> > >   */
> > > +static int attach_to_pi_state(u32 __user *uaddr, u32 uval,
> > > +			      struct futex_pi_state *pi_state,
> > >  			      struct futex_pi_state **ps)
> > >  {
> > >  	pid_t pid = uval & FUTEX_TID_MASK;
> > > +	int ret, uval2;
> > 
> > The uval should be an unsigned type:
> > 
> > u32 uval2;
> 
> Right you are.
> 
> > >  
> > >  	/*
> > >  	 * Userspace might have messed up non-PI and PI futexes [3]
> > > @@ -991,9 +1026,34 @@ static int attach_to_pi_state(u32 uval,
> > >  	if (unlikely(!pi_state))
> > >  		return -EINVAL;
> > >  
> > > +	/*
> > > +	 * We get here with hb->lock held, and having found a
> > > +	 * futex_top_waiter(). This means that futex_lock_pi() of said futex_q
> > > +	 * has dropped the hb->lock in between queue_me() and unqueue_me_pi(),
> > 
> > This context got here like this:
> > 
> > 	futex_lock_pi
> > 		hb lock
> > 		futex_lock_pi_atomic
> > 			top waiter
> > 			attach_to_pi_state()
> > 
> > 	The queue_me and unqueue_me_pi both come after this in futex_lock_pi.
> > 	Also, the hb lock is dropped in queue_me, not between queue_me and
> > 	unqueue_me_pi.
> > 
> > Are you saying that in order to be here, there are at least two tasks contending
> > for the lock, and one that has come before us has proceeded as far as queue_me()
> > but has not yet entered unqueue_me_pi(), therefor we know there is a waiter and
> > it has a pi_state? If so, I think we can make this much clearer by at least
> > noting the two tasks in play.
> 
> The point is that this other task must have a reference, and since we
> now hold hb->lock, it cannot go away.


OK, so yes, two tasks. Noting the two task contexts somewhere in that comment
block would make this easier to follow - which is why we're adding the comment.


> > > @@ -1336,6 +1418,7 @@ static int wake_futex_pi(u32 __user *uad
> > >  
> > >  	if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval)) {
> > >  		ret = -EFAULT;
> > > +
> > 
> > Stray whitespace addition? Not explicitly against coding-style, but I don't
> > normally see a new line before the closing brace leading to an else...
> 
> I found it more readable that way. Sod checkpatch and co ;-)

Heh, I didn't run checkpatch, just found it odd and unrelated. I hesitate
to call you on style and superfluous change - but hey, if I'd make the comment
to people contributing to platform drivers, it would be hypocritical not to do
the same for you :-) And, if the feedback doesn't apply at this level, then I
should drop it as a barrier for the platform drivers - so serves as a good
litmus test.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web