Messages in this thread | | | Date | Thu, 10 Dec 2015 14:09:48 +0100 | From | Peter Zijlstra <> | Subject | Re: [tip:locking/core] sched/wait: Fix signal handling in bit wait helpers |
| |
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
| |