Re: [PATCH, RFC] kthread: (possibly) a missing memory barrier in kthread_stop()

[Andy Whitcroft <apw@shadowen.org>]

On Tue, Feb 19, 2008 at 12:03:37AM +0100, Dmitry Adamushko wrote:
> 
> Hi,
> 
> 
> [ description ]
> 
> Subject: kthread: add a memory barrier to kthread_stop()
> 
> 'kthread' threads do a check in the following order:
> - set_current_state(TASK_INTERRUPTIBLE);
> - kthread_should_stop();
> 
> and set_current_state() implies an smp_mb().
> 
> on another side (kthread_stop), wake_up_process() does not seem to
> guarantee a full mb.
> 
> And 'kthread_stop_info.k' must be visible before wake_up_process()
> checks for/modifies a state of the 'kthread' task.
> 
> (the patch is at the end of the message)
> 
> 
> [ more detailed description ]
> 
> the current code might well be safe in case a to-be-stopped 'kthread'
> task is _not_ running on another CPU at the moment when kthread_stop()
> is called (in this case, 'rq->lock' will act as a kind of synch.
> point/barrier).
> 
> Another case is as follows:
> 
> CPU#0:
> 
> ...
> while (kthread_should_stop()) {
> 
>        if (condition)
>              schedule();
> 
>        /* ... do something useful ... */   <--- EIP
> 
>        set_current_state(TASK_INTERRUPTIBLE);
> }
> 
> so a 'kthread' task is about to call
> set_current_state(TASK_INTERRUPTIBLE) ...
> 
> 
> (in the mean time)
> 
> CPU#1:
> 
> kthread_stop()
> 
> -> kthread_stop_info.k = k                 (*)
> -> wake_up_process()
> 
> wake_up_process() looks like:
> 
> (try_to_wake_up)
> 
> IRQ_OFF
> LOCK
> 
> old_state = p->state;
> if (!(old_state & state))                  (**)
>          goto out;
> 
> ...
> 
> UNLOCK
> IRQ_ON
> 
> 
> let's suppose (*) and (**) are reordered
> (according to Documentation/memory-barriers.txt, neither IRQ_OFF nor
> LOCK may prevent it from happening).
> 
> - the state is TASK_RUNNING, so we are about to return.
> 
> - CPU#1 is about to execute (*) (it's guaranteed to be done before
> spin_unlock(&rq->lock) at the end of try_to_wake_up())
> 
> 
> (in the mean time)
> 
> CPU#0:
> 
> - set_current_state(TASK_INTERRUPTIBLE);
> - kthread_should_stop();
> 
> here, kthread_stop_info.k is not yet visible
> 
> - schedule()
> 
> ... 
> 
> we missed a 'kthread_stop' event.
> 
> hum?
> 
> 
> TIA,
> 
> ---
> 
> From: Dmitry Adamushko <dmitry.adamushko@gmail.com>
> Subject: kthread: add a memory barrier to kthread_stop()
> 
> 'kthread' threads do a check in the following order:
> - set_current_state(TASK_INTERRUPTIBLE);
> - kthread_should_stop();
> 
> and set_current_state() implies an smp_mb().
> 
> on another side (kthread_stop), wake_up_process() is not guaranteed to
> act as a full mb.
> 
> 'kthread_stop_info.k' must be visible before wake_up_process() checks
> for/modifies a state of the 'kthread' task.
> 
> 
> Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
> 
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 0ac8878..5167110 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -211,6 +211,10 @@ int kthread_stop(struct task_struct *k)
>  
>  	/* Now set kthread_should_stop() to true, and wake it up. */
>  	kthread_stop_info.k = k;
> +
> +	/* The previous store operation must not get ahead of the wakeup. */
> +	smp_mb();
> +
>  	wake_up_process(k);
>  	put_task_struct(k);

The rules as written do seem to support your theory.  The CPU has every
right to delay the .k = k as late as the UNLOCK operation.

On the read-side there is a full barrier implied by the
set_current_state(TASK_INTERRUPTIBLE), however this synchronises us with
the current global state, which may well not have the updated version
of .k.

That seems to imply that a write memory barrier would be sufficient to
cover this.

So three comments.  First, should this not be an smp_wmb.  Second, this
memory barrier is paired with the barrier in
set_current_state(TASK_INTERRUPTIBLE) and that probabally should be
documented as part of this patch.  Finally, I think the comment as is is
hard to understand I got the sense of it backwards on first reading;
perhaps something like this:

	/*
	 * Ensure kthread_stop_info.k is visible before wakeup, paired
	 * with barrier in set_current_state().
	 */

-apw