linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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).