Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751628AbbLJNJ4 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 10 Dec 2015 08:09:56 -0500
Received: from casper.infradead.org ([85.118.1.10]:47521 "EHLO
	casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750960AbbLJNJz (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 10 Dec 2015 08:09:55 -0500
Date: Thu, 10 Dec 2015 14:09:48 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: NeilBrown <nfbrown@novell.com>
Cc: torvalds@linux-foundation.org, tglx@linutronix.de,
        linux-kernel@vger.kernel.org, efault@gmx.de, mingo@kernel.org,
        hpa@zytor.com, vladimir.murzin@arm.com,
        linux-tip-commits@vger.kernel.org, jstancek@redhat.com,
        Oleg Nesterov <oleg@redhat.com>
Subject: Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait
 helpers
Message-ID: <20151210130948.GW6356@twins.programming.kicks-ass.net>
References: <20151201130404.GL3816@twins.programming.kicks-ass.net>
 <tip-68985633bccb6066bf1803e316fbc6c1f5b796d6@git.kernel.org>
 <20151208104712.GJ6356@twins.programming.kicks-ass.net>
 <87zixkph0m.fsf@notabene.neil.brown.name>
 <20151209074033.GF6357@twins.programming.kicks-ass.net>
 <87si3bpaxy.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87si3bpaxy.fsf@notabene.neil.brown.name>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2197
Lines: 72

On Thu, Dec 10, 2015 at 08:30:01AM +1100, NeilBrown wrote:
> On Wed, Dec 09 2015, Peter Zijlstra wrote:
> 
> > On Wed, Dec 09, 2015 at 12:06:33PM +1100, NeilBrown wrote:
> >> On Tue, Dec 08 2015, Peter Zijlstra wrote:
> >> 
> >> >>  
> >> >
> >> > *sigh*, so that patch was broken.. the below might fix it, but please
> >> > someone look at it, I seem to have a less than stellar track record
> >> > here...
> >> 
> >> This new change seems to be more intrusive than should be needed.
> >> Can't we just do:
> >> 
> >> 
> >>  __sched int bit_wait(struct wait_bit_key *word)
> >>  {
> >> +	long state = current->state;
> >
> > No, current->state can already be changed by this time.
> 
> Does that matter?
> It can only have changed to TASK_RUNNING - right?
> In that case signal_pending_state() will return 0 and the bit_wait() acts
> as though the thread was woken up normally (which it was) rather than by
> a signal (which maybe it was too, but maybe that happened just a tiny
> bit later).
> 
> As long as signal delivery doesn't change ->state, we should be safe.
> We should even be safe testing ->state *after* the call the schedule().

Blergh, all I've managed to far is to confuse myself further. Even
something like the original (+- the EINTR) should work when we consider
the looping, even when mixed with an occasional spurious wakeup.


int bit_wait()
{
	if (signal_pending_state(current->state, current))
		return -EINTR;
	schedule();
}


This can go wrong against raising a signal thusly:

	prepare_to_wait()
1:	if (signal_pending_state(current->state, current))
		// false, nothing pending
	schedule();
				set_tsk_thread_flag(t, TIF_SIGPENDING);

		<spurious wakeup>

	prepare_to_wait()
				wake_up_state(t, ...);
2:	if (signal_pending_state(current->state, current))
		// false, TASK_RUNNING

	schedule(); // doesn't block because pending

	prepare_to_wait()
3:	if (signal_pending_state(current->state, current))
		// true, pending


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/