From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756124AbdDHA4E (ORCPT <rfc822;w@1wt.eu>);
        Fri, 7 Apr 2017 20:56:04 -0400
Received: from bombadil.infradead.org ([65.50.211.133]:33413 "EHLO
        bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751034AbdDHAz5 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 7 Apr 2017 20:55:57 -0400
Date: Fri, 7 Apr 2017 17:55:43 -0700
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 11/13] futex: Rework futex_lock_pi() to use
 rt_mutex_*_proxy_lock()
Message-ID: <20170408005543.GA16143@fury>
References: <20170322103547.756091212@infradead.org>
 <20170322104152.062785528@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170322104152.062785528@infradead.org>
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Mar 22, 2017 at 11:35:58AM +0100, Peter Zijlstra wrote:
> By changing futex_lock_pi() to use rt_mutex_*_proxy_lock() we arrive
> at a point where all wait_list modifications are done under both
> hb->lock and wait_lock.
> 
> This closes the obvious interleave pattern between futex_lock_pi() and
> futex_unlock_pi(), but not entirely so. See below:
> 
> Before:
> 
> futex_lock_pi()			futex_unlock_pi()
>   unlock hb->lock
> 
> 				  lock hb->lock
> 				  unlock hb->lock
> 
> 				  lock rt_mutex->wait_lock
> 				  unlock rt_mutex_wait_lock
> 				    -EAGAIN
> 
>   lock rt_mutex->wait_lock
>   list_add
>   unlock rt_mutex->wait_lock
> 
>   schedule()
> 
>   lock rt_mutex->wait_lock
>   list_del
>   unlock rt_mutex->wait_lock
> 
> 				  <idem>
> 				    -EAGAIN
> 
>   lock hb->lock
> 
> 
> After:
> 
> futex_lock_pi()			futex_unlock_pi()
> 
>   lock hb->lock
>   lock rt_mutex->wait_lock
>   list_add
>   unlock rt_mutex->wait_lock
>   unlock hb->lock
> 
>   schedule()
> 				  lock hb->lock
> 				  unlock hb->lock
>   lock hb->lock
>   lock rt_mutex->wait_lock
>   list_del
>   unlock rt_mutex->wait_lock
> 
> 				  lock rt_mutex->wait_lock
> 				  unlock rt_mutex_wait_lock

Underscore to dereference:	  rt_mutex->wait_lock

> 				    -EAGAIN
> 
>   unlock hb->lock
> 
> 
> It does however solve the earlier starvation/live-lock scenario which
> got introduced with the -EAGAIN since unlike the before scenario;
> where the -EAGAIN happens while futex_unlock_pi() doesn't hold any
> locks; in the after scenario it happens while futex_unlock_pi()

I think you mean futex_lock_pi() here ----------^
And possibly in the previous reference, although both are true.

> actually holds a lock, and then we can serialize on that lock.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/futex.c                  |   70 +++++++++++++++++++++++++++-------------
>  kernel/locking/rtmutex.c        |   13 -------
>  kernel/locking/rtmutex_common.h |    1 
>  3 files changed, 48 insertions(+), 36 deletions(-)
> 
> Index: linux-2.6/kernel/futex.c

...

> @@ -2587,6 +2592,7 @@ static int futex_lock_pi(u32 __user *uad

...

> +no_block:
> +	/*
>  	 * Fixup the pi_state owner and possibly acquire the lock if we
>  	 * haven't already.
>  	 */

Deleted a bunch of commentary about the following comment and the code to follow
(which shows up just below this point). Turns out it isn't wrong... it's just
really complex. This snippet used to be self contained within the first if
block, and now the connection to the comment is less direct. I didn't come up
with a better way to say it though.... so just noting this here in case you or
someone else has a better idea.


        /*
         * If fixup_owner() faulted and was unable to handle the fault, unlock
         * it and return the fault to userspace.
         */
        if (ret && (rt_mutex_owner(&q.pi_state->pi_mutex) == current)) {
                pi_state = q.pi_state;
                get_pi_state(pi_state);
        }

        /* Unqueue and drop the lock */
        unqueue_me_pi(&q);

        if (pi_state) {
                rt_mutex_futex_unlock(&pi_state->pi_mutex);
                put_pi_state(pi_state);
        }

-- 
Darren Hart
VMware Open Source Technology Center