From: Manfred Spraul <manfred@colorfullife.com>
To: Waiman Long <longman@redhat.com>,
Davidlohr Bueso <dave@stgolabs.net>,
Peter Zijlstra <peterz@infradead.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, 1vier1@web.de
Subject: Re: [PATCH] ipc/sem: Fix race between to-be-woken task and waker
Date: Sun, 29 Sep 2019 12:24:31 +0200 [thread overview]
Message-ID: <d89b622a-2acf-b0a9-021d-c1c521a731f5@colorfullife.com> (raw)
In-Reply-To: <20190920155402.28996-1-longman () redhat ! com>
Hi Waiman,
I have now written the mail 3 times:
Twice I thought that I found a race, but during further analysis, it
always turns out that the spin_lock() is sufficient.
First, to avoid any obvious things: Until the series with e.g.
27d7be1801a4824e, there was a race inside sem_lock().
Thus it was possible that multiple threads were operating on the same
semaphore array, with obviously arbitrary impact.
On 9/20/19 5:54 PM, Waiman Long wrote:
>
> + /*
> + * A spurious wakeup at the right moment can cause race
> + * between the to-be-woken task and the waker leading to
> + * missed wakeup. Setting state back to TASK_INTERRUPTIBLE
> + * before checking queue.status will ensure that the race
> + * won't happen.
> + *
> + * CPU0 CPU1
> + *
> + * <spurious wakeup> wake_up_sem_queue_prepare():
> + * state = TASK_INTERRUPTIBLE status = error
> + * try_to_wake_up():
> + * smp_mb() smp_mb()
> + * if (status == -EINTR) if (!(p->state & state))
> + * schedule() goto out
> + */
> + set_current_state(TASK_INTERRUPTIBLE);
> +
So the the hypothesis is that we have a race due to the optimization
within try_to_wake_up():
If the status is already TASK_RUNNING, then the wakeup is a nop.
Correct?
The waker wants to use:
lock();
set_conditions();
unlock();
as the wake_q is a shared list, completely asynchroneously this will happen:
smp_mb(); //// ***1
if (current->state = TASK_INTERRUPTIBLE) current->state=TASK_RUNNING;
The only guarantee is that this will happen after lock(), it may happen
before set_conditions().
The task that goes to sleep uses:
lock();
check_conditions();
__set_current_state();
unlock(); //// ***2
schedule();
You propose to change that to:
lock();
set_current_state();
check_conditions();
unlock();
schedule();
I don't see a race anymore, and I don't see how the proposed change will
help.
e.g.: __set_current_state() and smp_mb() have paired memory barriers
***1 and ***2 above.
--
Manfred
next parent reply other threads:[~2019-09-29 10:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190920155402.28996-1-longman () redhat ! com>
2019-09-29 10:24 ` Manfred Spraul [this message]
2019-09-30 13:53 ` [PATCH] ipc/sem: Fix race between to-be-woken task and waker Waiman Long
2019-09-20 15:54 Waiman Long
2019-09-26 9:34 ` Peter Zijlstra
2019-09-26 18:12 ` Waiman Long
2019-09-27 4:59 ` Manfred Spraul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d89b622a-2acf-b0a9-021d-c1c521a731f5@colorfullife.com \
--to=manfred@colorfullife.com \
--cc=1vier1@web.de \
--cc=dave@stgolabs.net \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).