[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-kernel
Subject:    Re: [PATCH 1/7] sched,wait: Fix a kthread race with wait_woken()
From:       Oleg Nesterov <oleg () redhat ! com>
Date:       2014-10-31 22:13:40
Message-ID: 20141031221340.GA28563 () redhat ! com
[Download RAW message or body]

On 10/31, Peter Zijlstra wrote:
>
> There is a race between kthread_stop() and the new wait_woken() that
> can result in a lack of progress.

Likewise, the user of wait_woken() can miss any other event which is
not associated with wq we are going to sleep on. Please see below.

> +static inline bool is_kthread_should_stop(void)
> +{
> +	return (current->flags & PF_KTHREAD) && kthread_should_stop();
> +}
>
>  /*
>   * DEFINE_WAIT_FUNC(wait, woken_wake_func);
> @@ -326,7 +331,7 @@ long wait_woken(wait_queue_t *wait, unsi
>  	 * woken_wake_function() such that if we observe WQ_FLAG_WOKEN we must
>  	 * also observe all state before the wakeup.
>  	 */
> -	if (!(wait->flags & WQ_FLAG_WOKEN))
> +	if (!(wait->flags & WQ_FLAG_WOKEN) && !is_kthread_should_stop())
>  		timeout = schedule_timeout(timeout);

Well yes, this is more straightforward than other hacks we discussed before.
But see above, this doesn't look flexible enough.

And. This assumes that the user must also check kthread_should_stop(),
otherwise the waiting loop becomes a busy-wait loop.

So I won't argue, but I still think it would be better to allow the user to
do set_task_state() by hand if it needs to check the additional conditions.

Oleg.

--
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/
[prev in list] [next in list] [prev in thread] [next in thread]