From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754186AbbLTHhU (ORCPT ); Sun, 20 Dec 2015 02:37:20 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:42809 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754109AbbLTHhS (ORCPT ); Sun, 20 Dec 2015 02:37:18 -0500 Date: Sat, 19 Dec 2015 23:37:14 -0800 From: Darren Hart To: Mike Galbraith Cc: Thomas Gleixner , LKML , Ingo Molnar , Peter Zijlstra , Davidlohr Bueso , Bhuvanesh_Surachari@mentor.com, Andy Lowe Subject: Re: [patch 5/5] futex: Cleanup the goto confusion in requeue_pi() Message-ID: <20151220073714.GL7244@malice.jf.intel.com> References: <20151219200501.563704646@linutronix.de> <20151219200607.526665141@linutronix.de> <20151220051524.GH7244@malice.jf.intel.com> <1450590007.3296.6.camel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450590007.3296.6.camel@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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): >>From 2d4cce06d36b16dcd038d7a68a6efc7740848ee1 Mon Sep 17 00:00:00 2001 Message-Id: <2d4cce06d36b16dcd038d7a68a6efc7740848ee1.1450596757.git.dvhart@linux.intel.com> From: Darren Hart Date: Sat, 19 Dec 2015 22:57:05 -0800 Subject: [PATCH] futex: Add a get_pi_state wrapper For audit purposes, add an inline wrapper, get_pi_state(), around atomic_inc(&pi_state->refcount) to parallel the newly renamed put_pi_state(). To make the get/set parallel and more explicit, keep the refcount at 0 while in the cache and only inc to 1 when it is allocated to a waiter via alloc_pi_state(). Signed-off-by: Darren Hart --- kernel/futex.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index ae83ea7..4c71c86 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -706,7 +706,7 @@ static int refill_pi_state_cache(void) INIT_LIST_HEAD(&pi_state->list); /* pi_mutex gets initialized later */ pi_state->owner = NULL; - atomic_set(&pi_state->refcount, 1); + atomic_set(&pi_state->refcount, 0); pi_state->key = FUTEX_KEY_INIT; current->pi_state_cache = pi_state; @@ -714,12 +714,21 @@ static int refill_pi_state_cache(void) return 0; } +/* + * Adds a reference to the pi_state object. + */ +static inline void get_pi_state(struct futex_pi_state *pi_state) +{ + atomic_inc(&pi_state->refcount); +} + static struct futex_pi_state * alloc_pi_state(void) { struct futex_pi_state *pi_state = current->pi_state_cache; WARN_ON(!pi_state); current->pi_state_cache = NULL; + get_pi_state(pi_state); return pi_state; } @@ -756,10 +765,9 @@ static void put_pi_state(struct futex_pi_state *pi_state) /* * pi_state->list is already empty. * clear pi_state->owner. - * refcount is at 0 - put it back to 1. + * refcount is already at 0. */ pi_state->owner = NULL; - atomic_set(&pi_state->refcount, 1); current->pi_state_cache = pi_state; } } @@ -954,7 +962,7 @@ static int attach_to_pi_state(u32 uval, struct futex_pi_state *pi_state, if (pid != task_pid_vnr(pi_state->owner)) return -EINVAL; out_state: - atomic_inc(&pi_state->refcount); + get_pi_state(pi_state); *ps = pi_state; return 0; } @@ -1813,7 +1821,7 @@ retry_private: * refcount on the pi_state and store the pointer in * the futex_q object of the waiter. */ - atomic_inc(&pi_state->refcount); + get_pi_state(pi_state); this->pi_state = pi_state; ret = rt_mutex_start_proxy_lock(&pi_state->pi_mutex, this->rt_waiter, -- 2.1.4 -- Darren Hart Intel Open Source Technology Center