* [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
@ 2018-11-30 15:10 Prateek Sood
2018-12-03 6:38 ` Davidlohr Bueso
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Prateek Sood @ 2018-11-30 15:10 UTC (permalink / raw)
To: peterz, mingo, dbueso, prsood; +Cc: linux-kernel, sramana
In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
acquired for read operation by P1 using percpu_down_read().
Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
is in the process of acquiring cpu_hotplug_lock.
P1 P2
percpu_up_read() path percpu_down_write() path
rcu_sync_enter() //gp_state=GP_PASSED
rcu_sync_is_idle() //returns false down_write(rw_sem)
__percpu_up_read()
[L] task = rcu_dereference(w->task) //NULL
smp_rmb() [S] w->task = current
smp_mb()
[L] readers_active_check() //fails
schedule()
[S] __this_cpu_dec(read_count)
Since load of task can result in NULL. This can lead to missed wakeup
in rcuwait_wake_up(). Above sequence violated the following constraint
in rcuwait_wake_up():
WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk
This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering
of load before barrier with load and store after barrier for arm64
architecture. Here the requirement is to order store before smp_rmb()
with load after the smp_rmb().
For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier
(smp_mb) is required to complete the constraint of rcuwait_wake_up().
Signed-off-by: Prateek Sood <prsood@codeaurora.org>
---
kernel/exit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index f1d74f0..a10820d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
* MB (A) MB (B)
* [L] cond [L] tsk
*/
- smp_rmb(); /* (B) */
+ smp_mb(); /* (B) */
/*
* Avoid using task_rcu_dereference() magic as long as we are careful,
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-11-30 15:10 [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load Prateek Sood
@ 2018-12-03 6:38 ` Davidlohr Bueso
2018-12-03 19:36 ` Prateek Sood
2018-12-12 15:28 ` Andrea Parri
2019-01-21 11:25 ` [tip:locking/core] sched/wait: Fix rcuwait_wake_up() ordering tip-bot for Prateek Sood
2 siblings, 1 reply; 10+ messages in thread
From: Davidlohr Bueso @ 2018-12-03 6:38 UTC (permalink / raw)
To: Prateek Sood; +Cc: peterz, mingo, linux-kernel, sramana
On 2018-11-30 07:10, Prateek Sood wrote:
> In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
> acquired for read operation by P1 using percpu_down_read().
>
> Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
> is in the process of acquiring cpu_hotplug_lock.
>
> P1 P2
> percpu_up_read() path percpu_down_write() path
>
> rcu_sync_enter()
> //gp_state=GP_PASSED
>
> rcu_sync_is_idle() //returns false down_write(rw_sem)
>
> __percpu_up_read()
>
> [L] task = rcu_dereference(w->task) //NULL
>
> smp_rmb() [S] w->task = current
>
> smp_mb()
>
> [L] readers_active_check()
> //fails
> schedule()
>
> [S] __this_cpu_dec(read_count)
>
> Since load of task can result in NULL. This can lead to missed wakeup
> in rcuwait_wake_up(). Above sequence violated the following constraint
> in rcuwait_wake_up():
>
> WAIT WAKE
> [S] tsk = current [S] cond = true
> MB (A) MB (B)
> [L] cond [L] tsk
>
Hmm yeah we don't want rcu_wake_up() to get hoisted over the
__this_cpu_dec(read_count). The smp_rmb() does not make sense to me here
in the first place. Did you run into this scenario by code inspection or
you actually it the issue?
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-03 6:38 ` Davidlohr Bueso
@ 2018-12-03 19:36 ` Prateek Sood
2018-12-12 14:26 ` Prateek Sood
0 siblings, 1 reply; 10+ messages in thread
From: Prateek Sood @ 2018-12-03 19:36 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: peterz, mingo, linux-kernel, sramana
On 12/03/2018 12:08 PM, Davidlohr Bueso wrote:
> On 2018-11-30 07:10, Prateek Sood wrote:
>> In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
>> acquired for read operation by P1 using percpu_down_read().
>>
>> Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
>> is in the process of acquiring cpu_hotplug_lock.
>>
>> P1 P2
>> percpu_up_read() path percpu_down_write() path
>>
>> rcu_sync_enter() //gp_state=GP_PASSED
>>
>> rcu_sync_is_idle() //returns false down_write(rw_sem)
>>
>> __percpu_up_read()
>>
>> [L] task = rcu_dereference(w->task) //NULL
>>
>> smp_rmb() [S] w->task = current
>>
>> smp_mb()
>>
>> [L] readers_active_check() //fails
>> schedule()
>>
>> [S] __this_cpu_dec(read_count)
>>
>> Since load of task can result in NULL. This can lead to missed wakeup
>> in rcuwait_wake_up(). Above sequence violated the following constraint
>> in rcuwait_wake_up():
>>
>> WAIT WAKE
>> [S] tsk = current [S] cond = true
>> MB (A) MB (B)
>> [L] cond [L] tsk
>>
>
> Hmm yeah we don't want rcu_wake_up() to get hoisted over the __this_cpu_dec(read_count). The smp_rmb() does not make sense to me here in the first place. Did you run into this scenario by code inspection or you actually it the issue?
>
> Thanks,
> Davidlohr
I have checked one issue where it seems that cpu hotplug code
path is not able to get cpu_hotplug_lock in write mode and there
is a reader pending for cpu hotplug path to release
percpu_rw_semaphore->rwsem to acquire cpu_hotplug_lock.
This caused a deadlock.
From code inspection also it seems to be not adhering to arm64
smp_rmb() constraint of load/load-store ordering guarantee.
Thanks,
Prateek
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-03 19:36 ` Prateek Sood
@ 2018-12-12 14:26 ` Prateek Sood
2018-12-12 15:31 ` Davidlohr Bueso
0 siblings, 1 reply; 10+ messages in thread
From: Prateek Sood @ 2018-12-12 14:26 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: peterz, mingo, linux-kernel, sramana
On 12/04/2018 01:06 AM, Prateek Sood wrote:
> On 12/03/2018 12:08 PM, Davidlohr Bueso wrote:
>> On 2018-11-30 07:10, Prateek Sood wrote:
>>> In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
>>> acquired for read operation by P1 using percpu_down_read().
>>>
>>> Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
>>> is in the process of acquiring cpu_hotplug_lock.
>>>
>>> P1 P2
>>> percpu_up_read() path percpu_down_write() path
>>>
>>> rcu_sync_enter() //gp_state=GP_PASSED
>>>
>>> rcu_sync_is_idle() //returns false down_write(rw_sem)
>>>
>>> __percpu_up_read()
>>>
>>> [L] task = rcu_dereference(w->task) //NULL
>>>
>>> smp_rmb() [S] w->task = current
>>>
>>> smp_mb()
>>>
>>> [L] readers_active_check() //fails
>>> schedule()
>>>
>>> [S] __this_cpu_dec(read_count)
>>>
>>> Since load of task can result in NULL. This can lead to missed wakeup
>>> in rcuwait_wake_up(). Above sequence violated the following constraint
>>> in rcuwait_wake_up():
>>>
>>> WAIT WAKE
>>> [S] tsk = current [S] cond = true
>>> MB (A) MB (B)
>>> [L] cond [L] tsk
>>>
>>
>> Hmm yeah we don't want rcu_wake_up() to get hoisted over the __this_cpu_dec(read_count). The smp_rmb() does not make sense to me here in the first place. Did you run into this scenario by code inspection or you actually it the issue?
>>
>> Thanks,
>> Davidlohr
>
> I have checked one issue where it seems that cpu hotplug code
> path is not able to get cpu_hotplug_lock in write mode and there
> is a reader pending for cpu hotplug path to release
> percpu_rw_semaphore->rwsem to acquire cpu_hotplug_lock.
> This caused a deadlock.
>
> From code inspection also it seems to be not adhering to arm64
> smp_rmb() constraint of load/load-store ordering guarantee.
>
>
> Thanks,
> Prateek
>
Folks,
Please confirm if the suspicion of smp_rmb is correct.
IMO, it should be smp_mb() translating to dmb ish.
Thanks
Prateek
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-11-30 15:10 [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load Prateek Sood
2018-12-03 6:38 ` Davidlohr Bueso
@ 2018-12-12 15:28 ` Andrea Parri
2018-12-21 6:35 ` Prateek Sood
2019-01-21 11:25 ` [tip:locking/core] sched/wait: Fix rcuwait_wake_up() ordering tip-bot for Prateek Sood
2 siblings, 1 reply; 10+ messages in thread
From: Andrea Parri @ 2018-12-12 15:28 UTC (permalink / raw)
To: Prateek Sood; +Cc: peterz, mingo, dbueso, linux-kernel, sramana
On Fri, Nov 30, 2018 at 08:40:56PM +0530, Prateek Sood wrote:
> In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
> acquired for read operation by P1 using percpu_down_read().
>
> Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
> is in the process of acquiring cpu_hotplug_lock.
>
> P1 P2
> percpu_up_read() path percpu_down_write() path
>
> rcu_sync_enter() //gp_state=GP_PASSED
>
> rcu_sync_is_idle() //returns false down_write(rw_sem)
>
> __percpu_up_read()
>
> [L] task = rcu_dereference(w->task) //NULL
>
> smp_rmb() [S] w->task = current
>
> smp_mb()
>
> [L] readers_active_check() //fails
> schedule()
>
> [S] __this_cpu_dec(read_count)
>
> Since load of task can result in NULL. This can lead to missed wakeup
> in rcuwait_wake_up(). Above sequence violated the following constraint
> in rcuwait_wake_up():
>
> WAIT WAKE
> [S] tsk = current [S] cond = true
> MB (A) MB (B)
> [L] cond [L] tsk
>
> This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering
> of load before barrier with load and store after barrier for arm64
> architecture. Here the requirement is to order store before smp_rmb()
> with load after the smp_rmb().
>
> For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier
> (smp_mb) is required to complete the constraint of rcuwait_wake_up().
>
> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
I know this is going to sound ridiculous (coming from me or from
the Italian that I am), but it looks like we could both work on
our English. ;-)
But the fix seems correct to me:
Reviewed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
It might be a good idea to integrate this fix with fixes to the
inline comments/annotations: for example, I see that the comment
in rcuwait_wake_up() mentions a non-existing rcuwait_trywake();
moreover, the memory-barrier annotation "B" is used also for the
smp_mb() preceding the __this_cpu_dec() in __percpu_up_read().
Andrea
> ---
> kernel/exit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index f1d74f0..a10820d 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
> * MB (A) MB (B)
> * [L] cond [L] tsk
> */
> - smp_rmb(); /* (B) */
> + smp_mb(); /* (B) */
>
> /*
> * Avoid using task_rcu_dereference() magic as long as we are careful,
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-12 14:26 ` Prateek Sood
@ 2018-12-12 15:31 ` Davidlohr Bueso
2018-12-21 7:29 ` Prateek Sood
0 siblings, 1 reply; 10+ messages in thread
From: Davidlohr Bueso @ 2018-12-12 15:31 UTC (permalink / raw)
To: Prateek Sood; +Cc: peterz, mingo, linux-kernel, sramana
On 2018-12-12 06:26, Prateek Sood wrote:
> Please confirm if the suspicion of smp_rmb is correct.
> IMO, it should be smp_mb() translating to dmb ish.
Feel free to add my ack. This should also be Cc to stable as of v4.11.
Fixes: 8f95c90ceb54 (sched/wait, RCU: Introduce rcuwait machinery)
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-12 15:28 ` Andrea Parri
@ 2018-12-21 6:35 ` Prateek Sood
0 siblings, 0 replies; 10+ messages in thread
From: Prateek Sood @ 2018-12-21 6:35 UTC (permalink / raw)
To: Andrea Parri; +Cc: peterz, mingo, dbueso, linux-kernel, sramana
On 12/12/2018 08:58 PM, Andrea Parri wrote:
> On Fri, Nov 30, 2018 at 08:40:56PM +0530, Prateek Sood wrote:
>> In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already
>> acquired for read operation by P1 using percpu_down_read().
>>
>> Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2
>> is in the process of acquiring cpu_hotplug_lock.
>>
>> P1 P2
>> percpu_up_read() path percpu_down_write() path
>>
>> rcu_sync_enter() //gp_state=GP_PASSED
>>
>> rcu_sync_is_idle() //returns false down_write(rw_sem)
>>
>> __percpu_up_read()
>>
>> [L] task = rcu_dereference(w->task) //NULL
>>
>> smp_rmb() [S] w->task = current
>>
>> smp_mb()
>>
>> [L] readers_active_check() //fails
>> schedule()
>>
>> [S] __this_cpu_dec(read_count)
>>
>> Since load of task can result in NULL. This can lead to missed wakeup
>> in rcuwait_wake_up(). Above sequence violated the following constraint
>> in rcuwait_wake_up():
>>
>> WAIT WAKE
>> [S] tsk = current [S] cond = true
>> MB (A) MB (B)
>> [L] cond [L] tsk
>>
>> This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering
>> of load before barrier with load and store after barrier for arm64
>> architecture. Here the requirement is to order store before smp_rmb()
>> with load after the smp_rmb().
>>
>> For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier
>> (smp_mb) is required to complete the constraint of rcuwait_wake_up().
>>
>> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
>
> I know this is going to sound ridiculous (coming from me or from
> the Italian that I am), but it looks like we could both work on
> our English. ;-)
>
> But the fix seems correct to me:
>
> Reviewed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
>
> It might be a good idea to integrate this fix with fixes to the
> inline comments/annotations: for example, I see that the comment
> in rcuwait_wake_up() mentions a non-existing rcuwait_trywake();
Ok, I will update the comment in next version of the patch.
> moreover, the memory-barrier annotation "B" is used also for the
> smp_mb() preceding the __this_cpu_dec() in __percpu_up_read().
In this annotation "B" is corresponding to annotation "A" in
rcuwait_wait_event(). So this seems to be correct.
>
> Andrea
>
>
>> ---
>> kernel/exit.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/exit.c b/kernel/exit.c
>> index f1d74f0..a10820d 100644
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
>> * MB (A) MB (B)
>> * [L] cond [L] tsk
>> */
>> - smp_rmb(); /* (B) */
>> + smp_mb(); /* (B) */
>>
>> /*
>> * Avoid using task_rcu_dereference() magic as long as we are careful,
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
>> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-12 15:31 ` Davidlohr Bueso
@ 2018-12-21 7:29 ` Prateek Sood
2018-12-21 9:45 ` Andrea Parri
0 siblings, 1 reply; 10+ messages in thread
From: Prateek Sood @ 2018-12-21 7:29 UTC (permalink / raw)
To: dbueso, andrea.parri, peterz, mingo; +Cc: linux-kernel, sramana, Prateek Sood
P1 is releaseing the cpu_hotplug_lock and P2 is acquiring
cpu_hotplug_lock.
P1 P2
percpu_up_read() path percpu_down_write() path
rcu_sync_enter() //gp_state=GP_PASSED
rcu_sync_is_idle() //returns false down_write(rw_sem)
__percpu_up_read()
[L] task = rcu_dereference(w->task) //NULL
smp_rmb() [S] w->task = current
smp_mb()
[L] readers_active_check() //fails
schedule()
[S] __this_cpu_dec(read_count)
Since load of task can result in NULL, it can lead to missed wakeup
in rcuwait_wake_up(). Above sequence violated the following constraint
in rcuwait_wake_up():
WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk
This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering
of load before barrier with load and store after barrier for arm64
architecture. Here the requirement is to order store before smp_rmb()
with load after the smp_rmb().
For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier
(smp_mb) is required to complete the constraint of rcuwait_wake_up().
Signed-off-by: Prateek Sood <prsood@codeaurora.org>
Acked-by: Davidlohr Bueso <dbueso@suse.de>
---
kernel/exit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index ac1a814..696e0e1 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -298,7 +298,7 @@ void rcuwait_wake_up(struct rcuwait *w)
/*
* Order condition vs @task, such that everything prior to the load
* of @task is visible. This is the condition as to why the user called
- * rcuwait_trywake() in the first place. Pairs with set_current_state()
+ * rcuwait_wake_up() in the first place. Pairs with set_current_state()
* barrier (A) in rcuwait_wait_event().
*
* WAIT WAKE
@@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
* MB (A) MB (B)
* [L] cond [L] tsk
*/
- smp_rmb(); /* (B) */
+ smp_mb(); /* (B) */
/*
* Avoid using task_rcu_dereference() magic as long as we are careful,
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load
2018-12-21 7:29 ` Prateek Sood
@ 2018-12-21 9:45 ` Andrea Parri
0 siblings, 0 replies; 10+ messages in thread
From: Andrea Parri @ 2018-12-21 9:45 UTC (permalink / raw)
To: Prateek Sood; +Cc: dbueso, peterz, mingo, linux-kernel, sramana
On Fri, Dec 21, 2018 at 12:59:13PM +0530, Prateek Sood wrote:
> P1 is releaseing the cpu_hotplug_lock and P2 is acquiring
> cpu_hotplug_lock.
>
> P1 P2
> percpu_up_read() path percpu_down_write() path
>
> rcu_sync_enter() //gp_state=GP_PASSED
>
> rcu_sync_is_idle() //returns false down_write(rw_sem)
>
> __percpu_up_read()
>
> [L] task = rcu_dereference(w->task) //NULL
>
> smp_rmb() [S] w->task = current
>
> smp_mb()
>
> [L] readers_active_check() //fails
> schedule()
>
> [S] __this_cpu_dec(read_count)
>
> Since load of task can result in NULL, it can lead to missed wakeup
> in rcuwait_wake_up(). Above sequence violated the following constraint
> in rcuwait_wake_up():
>
> WAIT WAKE
> [S] tsk = current [S] cond = true
> MB (A) MB (B)
> [L] cond [L] tsk
>
> This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering
> of load before barrier with load and store after barrier for arm64
> architecture. Here the requirement is to order store before smp_rmb()
> with load after the smp_rmb().
>
> For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier
> (smp_mb) is required to complete the constraint of rcuwait_wake_up().
>
> Signed-off-by: Prateek Sood <prsood@codeaurora.org>
> Acked-by: Davidlohr Bueso <dbueso@suse.de>
It looks like Peter has already queued this, c.f.,
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=locking/core&id=73685b8af253cf32b1b41b3045f2828c6fb2439e
with a modified changelog and my Reviewed-by (that I confirm).
I can't tell how/when this is going to be upstreamed (guess via -tip),
Peter?
Andrea
>
> ---
> kernel/exit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index ac1a814..696e0e1 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -298,7 +298,7 @@ void rcuwait_wake_up(struct rcuwait *w)
> /*
> * Order condition vs @task, such that everything prior to the load
> * of @task is visible. This is the condition as to why the user called
> - * rcuwait_trywake() in the first place. Pairs with set_current_state()
> + * rcuwait_wake_up() in the first place. Pairs with set_current_state()
> * barrier (A) in rcuwait_wait_event().
> *
> * WAIT WAKE
> @@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
> * MB (A) MB (B)
> * [L] cond [L] tsk
> */
> - smp_rmb(); /* (B) */
> + smp_mb(); /* (B) */
>
> /*
> * Avoid using task_rcu_dereference() magic as long as we are careful,
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:locking/core] sched/wait: Fix rcuwait_wake_up() ordering
2018-11-30 15:10 [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load Prateek Sood
2018-12-03 6:38 ` Davidlohr Bueso
2018-12-12 15:28 ` Andrea Parri
@ 2019-01-21 11:25 ` tip-bot for Prateek Sood
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Prateek Sood @ 2019-01-21 11:25 UTC (permalink / raw)
To: linux-tip-commits
Cc: prsood, mingo, linux-kernel, dbueso, torvalds, tglx, andrea.parri,
hpa, peterz
Commit-ID: 6dc080eeb2ba01973bfff0d79844d7a59e12542e
Gitweb: https://git.kernel.org/tip/6dc080eeb2ba01973bfff0d79844d7a59e12542e
Author: Prateek Sood <prsood@codeaurora.org>
AuthorDate: Fri, 30 Nov 2018 20:40:56 +0530
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Jan 2019 11:15:36 +0100
sched/wait: Fix rcuwait_wake_up() ordering
For some peculiar reason rcuwait_wake_up() has the right barrier in
the comment, but not in the code.
This mistake has been observed to cause a deadlock in the following
situation:
P1 P2
percpu_up_read() percpu_down_write()
rcu_sync_is_idle() // false
rcu_sync_enter()
...
__percpu_up_read()
[S] ,- __this_cpu_dec(*sem->read_count)
| smp_rmb();
[L] | task = rcu_dereference(w->task) // NULL
|
| [S] w->task = current
| smp_mb();
| [L] readers_active_check() // fail
`-> <store happens here>
Where the smp_rmb() (obviously) fails to constrain the store.
[ peterz: Added changelog. ]
Signed-off-by: Prateek Sood <prsood@codeaurora.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Acked-by: Davidlohr Bueso <dbueso@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 8f95c90ceb54 ("sched/wait, RCU: Introduce rcuwait machinery")
Link: https://lkml.kernel.org/r/1543590656-7157-1-git-send-email-prsood@codeaurora.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/exit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 284f2fe9a293..3fb7be001964 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -307,7 +307,7 @@ void rcuwait_wake_up(struct rcuwait *w)
* MB (A) MB (B)
* [L] cond [L] tsk
*/
- smp_rmb(); /* (B) */
+ smp_mb(); /* (B) */
/*
* Avoid using task_rcu_dereference() magic as long as we are careful,
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-21 11:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-30 15:10 [PATCH] percpu_rwsem: fix missed wakeup due to reordering of load Prateek Sood
2018-12-03 6:38 ` Davidlohr Bueso
2018-12-03 19:36 ` Prateek Sood
2018-12-12 14:26 ` Prateek Sood
2018-12-12 15:31 ` Davidlohr Bueso
2018-12-21 7:29 ` Prateek Sood
2018-12-21 9:45 ` Andrea Parri
2018-12-12 15:28 ` Andrea Parri
2018-12-21 6:35 ` Prateek Sood
2019-01-21 11:25 ` [tip:locking/core] sched/wait: Fix rcuwait_wake_up() ordering tip-bot for Prateek Sood
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.