All of lore.kernel.org
 help / color / mirror / Atom feed
From: Darren Hart <dvhart@infradead.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: tglx@linutronix.de, mingo@kernel.org, juri.lelli@arm.com,
	rostedt@goodmis.org, xlpang@redhat.com, bigeasy@linutronix.de,
	linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
	jdesfossez@efficios.com, bristot@redhat.com
Subject: Re: [PATCH -v6 10/13] futex,rt_mutex: Restructure rt_mutex_finish_proxy_lock()
Date: Fri, 7 Apr 2017 16:30:59 -0700	[thread overview]
Message-ID: <20170407233059.GA14673@fury> (raw)
In-Reply-To: <20170322104152.001659630@infradead.org>

On Wed, Mar 22, 2017 at 11:35:57AM +0100, Peter Zijlstra wrote:
> With the ultimate goal of keeping rt_mutex wait_list and futex_q
> waiters consistent we want to split 'rt_mutex_futex_lock()' into finer

I want to be clear that I understand why this patch is needed - as it actually
moves both the waiter removal and the rt_waiter freeing under the hb lock while
you've been working to be less dependent on the hb lock.

Was inconsistency of the rt_mutex wait_list and the futex_q waiters a problem
before this patch series, or do the previous patches make this one necessary?

It makes sense that for the two to be consistent they should be manipulated
under a common lock.

> parts, such that only the actual blocking can be done without hb->lock
> held.
> 
> This means we need to split rt_mutex_finish_proxy_lock() into two
> parts, one that does the blocking and one that does remove_waiter()
> when we fail to acquire.
> 
> When we do acquire, we can safely remove ourselves, since there is no
> concurrency on the lock owner.
> 
> This means that, except for futex_lock_pi(), all wait_list
> modifications are done with both hb->lock and wait_lock held.
> 
> [bigeasy@linutronix.de: fix for futex_requeue_pi_signal_restart]
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/futex.c                  |    7 +++--
>  kernel/locking/rtmutex.c        |   53 ++++++++++++++++++++++++++++++++++------
>  kernel/locking/rtmutex_common.h |    8 +++---
>  3 files changed, 56 insertions(+), 12 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3032,10 +3032,13 @@ static int futex_wait_requeue_pi(u32 __u
>  		 */
>  		WARN_ON(!q.pi_state);
>  		pi_mutex = &q.pi_state->pi_mutex;
> -		ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter);
> -		debug_rt_mutex_free_waiter(&rt_waiter);
> +		ret = rt_mutex_wait_proxy_lock(pi_mutex, to, &rt_waiter);
>  
>  		spin_lock(q.lock_ptr);
> +		if (ret && !rt_mutex_cleanup_proxy_lock(pi_mutex, &rt_waiter))
> +			ret = 0;
> +
> +		debug_rt_mutex_free_waiter(&rt_waiter);
>  		/*
>  		 * Fixup the pi_state owner and possibly acquire the lock if we
>  		 * haven't already.
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1753,21 +1753,23 @@ struct task_struct *rt_mutex_next_owner(
>  }
>  
>  /**
> - * rt_mutex_finish_proxy_lock() - Complete lock acquisition
> + * rt_mutex_wait_proxy_lock() - Wait for lock acquisition
>   * @lock:		the rt_mutex we were woken on
>   * @to:			the timeout, null if none. hrtimer should already have
>   *			been started.
>   * @waiter:		the pre-initialized rt_mutex_waiter
>   *
> - * Complete the lock acquisition started our behalf by another thread.
> + * Wait for the the lock acquisition started on our behalf by
> + * rt_mutex_start_proxy_lock(). Upon failure, the caller must call
> + * rt_mutex_cleanup_proxy_lock().
>   *
>   * Returns:
>   *  0 - success
>   * <0 - error, one of -EINTR, -ETIMEDOUT
>   *
> - * Special API call for PI-futex requeue support
> + * Special API call for PI-futex support
>   */
> -int rt_mutex_finish_proxy_lock(struct rt_mutex *lock,
> +int rt_mutex_wait_proxy_lock(struct rt_mutex *lock,
>  			       struct hrtimer_sleeper *to,
>  			       struct rt_mutex_waiter *waiter)
>  {
> @@ -1780,9 +1782,6 @@ int rt_mutex_finish_proxy_lock(struct rt
>  	/* sleep on the mutex */
>  	ret = __rt_mutex_slowlock(lock, TASK_INTERRUPTIBLE, to, waiter);
>  
> -	if (unlikely(ret))
> -		remove_waiter(lock, waiter);
> -
>  	/*
>  	 * try_to_take_rt_mutex() sets the waiter bit unconditionally. We might
>  	 * have to fix that up.
> @@ -1793,3 +1792,43 @@ int rt_mutex_finish_proxy_lock(struct rt
>  
>  	return ret;
>  }
> +
> +/**
> + * rt_mutex_cleanup_proxy_lock() - Cleanup failed lock acquisition
> + * @lock:		the rt_mutex we were woken on
> + * @waiter:		the pre-initialized rt_mutex_waiter
> + *
> + * Attempt to clean up after a failed rt_mutex_wait_proxy_lock().
> + *
> + * Unless we acquired the lock; we're still enqueued on the wait-list and can
> + * in fact still be granted ownership until we're removed. Therefore we can
> + * find we are in fact the owner and must disregard the
> + * rt_mutex_wait_proxy_lock() failure.
> + *
> + * Returns:
> + *  true  - did the cleanup, we done.
> + *  false - we acquired the lock after rt_mutex_wait_proxy_lock() returned,
> + *          caller should disregards its return value.
> + *
> + * Special API call for PI-futex support
> + */
> +bool rt_mutex_cleanup_proxy_lock(struct rt_mutex *lock,
> +				 struct rt_mutex_waiter *waiter)
> +{
> +	bool cleanup = false;
> +
> +	raw_spin_lock_irq(&lock->wait_lock);
> +	/*
> +	 * Unless we're the owner; we're still enqueued on the wait_list.
> +	 * So check if we became owner, if not, take us off the wait_list.
> +	 */
> +	if (rt_mutex_owner(lock) != current) {
> +		remove_waiter(lock, waiter);
> +		fixup_rt_mutex_waiters(lock);
> +		cleanup = true;
> +	}
> +	raw_spin_unlock_irq(&lock->wait_lock);
> +
> +	return cleanup;
> +}
> +
> --- a/kernel/locking/rtmutex_common.h
> +++ b/kernel/locking/rtmutex_common.h
> @@ -107,9 +107,11 @@ extern void rt_mutex_init_waiter(struct
>  extern int rt_mutex_start_proxy_lock(struct rt_mutex *lock,
>  				     struct rt_mutex_waiter *waiter,
>  				     struct task_struct *task);
> -extern int rt_mutex_finish_proxy_lock(struct rt_mutex *lock,
> -				      struct hrtimer_sleeper *to,
> -				      struct rt_mutex_waiter *waiter);
> +extern int rt_mutex_wait_proxy_lock(struct rt_mutex *lock,
> +			       struct hrtimer_sleeper *to,
> +			       struct rt_mutex_waiter *waiter);
> +extern bool rt_mutex_cleanup_proxy_lock(struct rt_mutex *lock,
> +				 struct rt_mutex_waiter *waiter);
>  
>  extern int rt_mutex_timed_futex_lock(struct rt_mutex *l, struct hrtimer_sleeper *to);
>  extern int rt_mutex_futex_trylock(struct rt_mutex *l);
> 
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

  parent reply	other threads:[~2017-04-07 23:31 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-22 10:35 [PATCH -v6 00/13] The arduous story of FUTEX_UNLOCK_PI Peter Zijlstra
2017-03-22 10:35 ` [PATCH -v6 01/13] futex: Cleanup variable names for futex_top_waiter() Peter Zijlstra
2017-03-23 18:19   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:11   ` [PATCH -v6 01/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 02/13] futex: Use smp_store_release() in mark_wake_futex() Peter Zijlstra
2017-03-23 18:19   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:16   ` [PATCH -v6 02/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 03/13] futex: Remove rt_mutex_deadlock_account_*() Peter Zijlstra
2017-03-23 18:20   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-24 21:29   ` [PATCH -v6 03/13] " Darren Hart
2017-03-24 21:31     ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt_mutex API Peter Zijlstra
2017-03-23 18:20   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-03-25  0:37   ` [PATCH -v6 04/13] " Darren Hart
2017-04-06 12:15     ` Peter Zijlstra
2017-04-06 17:02       ` Darren Hart
2017-04-05 15:02   ` Darren Hart
2017-04-06 12:17     ` Peter Zijlstra
2017-04-06 17:08       ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 05/13] futex: Change locking rules Peter Zijlstra
2017-03-23 18:21   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:18   ` [PATCH -v6 05/13] " Darren Hart
2017-04-06 12:28     ` Peter Zijlstra
2017-04-06 15:58       ` Joe Perches
2017-04-06 17:21       ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 06/13] futex: Cleanup refcounting Peter Zijlstra
2017-03-23 18:21   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:29   ` [PATCH -v6 06/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 07/13] futex: Rework inconsistent rt_mutex/futex_q state Peter Zijlstra
2017-03-23 18:22   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 21:58   ` [PATCH -v6 07/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock Peter Zijlstra
2017-03-23 18:22   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 23:52   ` [PATCH -v6 08/13] " Darren Hart
2017-04-06 12:42     ` Peter Zijlstra
2017-04-06 17:42       ` Darren Hart
2017-03-22 10:35 ` [PATCH -v6 09/13] futex,rt_mutex: Introduce rt_mutex_init_waiter() Peter Zijlstra
2017-03-23 18:23   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-05 23:57   ` [PATCH -v6 09/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 10/13] futex,rt_mutex: Restructure rt_mutex_finish_proxy_lock() Peter Zijlstra
2017-03-23 18:23   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-07 23:30   ` Darren Hart [this message]
2017-04-07 23:35     ` [PATCH -v6 10/13] " Darren Hart
2017-03-22 10:35 ` [PATCH -v6 11/13] futex: Rework futex_lock_pi() to use rt_mutex_*_proxy_lock() Peter Zijlstra
2017-03-23 18:24   ` [tip:locking/core] " tip-bot for Peter Zijlstra
2017-04-08  0:55   ` [PATCH -v6 11/13] " Darren Hart
2017-04-10 15:51   ` alexander.levin
2017-04-10 16:03     ` Thomas Gleixner
2017-04-14  9:30       ` [tip:locking/core] futex: Avoid freeing an active timer tip-bot for Thomas Gleixner
2017-03-22 10:35 ` [PATCH -v6 12/13] futex: futex_unlock_pi() determinism Peter Zijlstra
2017-03-23 18:24   ` [tip:locking/core] futex: Futex_unlock_pi() determinism tip-bot for Peter Zijlstra
2017-04-08  1:27   ` [PATCH -v6 12/13] futex: futex_unlock_pi() determinism Darren Hart
2017-03-22 10:36 ` [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Peter Zijlstra
2017-03-23 18:25   ` [tip:locking/core] futex: Drop hb->lock before enqueueing on the rtmutex tip-bot for Peter Zijlstra
2017-04-08  2:26   ` [PATCH -v6 13/13] futex: futex_lock_pi() vs PREEMPT_RT_FULL Darren Hart
2017-04-08  5:22     ` Mike Galbraith
2017-04-10  8:43     ` Sebastian Andrzej Siewior
2017-04-10  9:08     ` Peter Zijlstra
2017-04-10 16:05       ` Darren Hart
2017-03-24  1:45 ` [PATCH -v6 00/13] The arduous story of FUTEX_UNLOCK_PI Darren Hart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170407233059.GA14673@fury \
    --to=dvhart@infradead.org \
    --cc=bigeasy@linutronix.de \
    --cc=bristot@redhat.com \
    --cc=jdesfossez@efficios.com \
    --cc=juri.lelli@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=xlpang@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.