diff mbox

[2/3] ath10k: Grab rcu_read_lock before the txqs spinlock.

Message ID 1471569995-10028-2-git-send-email-greearb@candelatech.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Ben Greear Aug. 19, 2016, 1:26 a.m. UTC
From: Ben Greear <greearb@candelatech.com>

I was seeing some spin-lock hangs in this area of the code,
and it seems more proper to do the rcu-read-lock outside of
the spin lock.  I am not sure how much this matters, however.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rajkumar Manoharan Aug. 19, 2016, 3:01 a.m. UTC | #1
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 916119c..d96c06e 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>         int max;
>         int loop_max = 2000;
> 
> -       spin_lock_bh(&ar->txqs_lock);
>         rcu_read_lock();
> +       spin_lock_bh(&ar->txqs_lock);
>
Ben,

It is quite possible that push_pending can be preempted after acquiring rcu_lock which
may lead to deadlock. no? I assume to prevent that spin_lock is taken first. 

Could you please explain how this reordering is fixing dead lock?

-Rajkumar
Ben Greear Aug. 19, 2016, 3:28 a.m. UTC | #2
On 08/18/2016 08:01 PM, Manoharan, Rajkumar wrote:
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 916119c..d96c06e 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>>          int max;
>>          int loop_max = 2000;
>>
>> -       spin_lock_bh(&ar->txqs_lock);
>>          rcu_read_lock();
>> +       spin_lock_bh(&ar->txqs_lock);
>>
> Ben,
>
> It is quite possible that push_pending can be preempted after acquiring rcu_lock which
> may lead to deadlock. no? I assume to prevent that spin_lock is taken first.
>
> Could you please explain how this reordering is fixing dead lock?

It did not obviously fix the spin lock issue, but the issue went away.  Maybe it
was because I fixed the stale memory access issues at around the same time.

But, I don't think you can deadlock by taking rcu lock first and then the spinlock.

I checked all places where the spinlock is held, and I do not see any way for any of
them to block forever (In my kernel, I have a 2000 time limit on spinning in the push pending
method, which can help make sure we don't spin forever).

http://dmz2.candelatech.com/?p=linux-4.7.dev.y/.git;a=commitdiff;h=5d192657269d8e20fce733f894bb1b68df71db00

I was also worried that some calls from mac80211 might be holding rcu_read_lock while calling into
ath10k code that would grab the spinlock.  If that is the case (and I didn't verify it was), then
you could have a lock inversion by taking spinlock before rcu read lock in the push-pending method.

Anyway, someone that understands locking subtleties better might have more clue about this code
than I do.

Thanks,
Ben
Kalle Valo Sept. 9, 2016, 1:36 p.m. UTC | #3
greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> I was seeing some spin-lock hangs in this area of the code,
> and it seems more proper to do the rcu-read-lock outside of
> the spin lock.  I am not sure how much this matters, however.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 916119c..d96c06e 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>  	int max;
>  	int loop_max = 2000;
>  
> -	spin_lock_bh(&ar->txqs_lock);
>  	rcu_read_lock();
> +	spin_lock_bh(&ar->txqs_lock);
>  
>  	last = list_last_entry(&ar->txqs, struct ath10k_txq, list);
>  	while (!list_empty(&ar->txqs)) {
> @@ -4342,8 +4342,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>  			break;
>  	}
>  
> -	rcu_read_unlock();
>  	spin_unlock_bh(&ar->txqs_lock);
> +	rcu_read_unlock();

I'm no RCU expert but this isn't making any sense. Maybe it changes
timings on your kernel so that it hides the real problem?
Ben Greear Sept. 9, 2016, 2:47 p.m. UTC | #4
On 09/09/2016 06:36 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> I was seeing some spin-lock hangs in this area of the code,
>> and it seems more proper to do the rcu-read-lock outside of
>> the spin lock.  I am not sure how much this matters, however.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   drivers/net/wireless/ath/ath10k/mac.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 916119c..d96c06e 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>>   	int max;
>>   	int loop_max = 2000;
>>
>> -	spin_lock_bh(&ar->txqs_lock);
>>   	rcu_read_lock();
>> +	spin_lock_bh(&ar->txqs_lock);
>>
>>   	last = list_last_entry(&ar->txqs, struct ath10k_txq, list);
>>   	while (!list_empty(&ar->txqs)) {
>> @@ -4342,8 +4342,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar)
>>   			break;
>>   	}
>>
>> -	rcu_read_unlock();
>>   	spin_unlock_bh(&ar->txqs_lock);
>> +	rcu_read_unlock();
>
> I'm no RCU expert but this isn't making any sense. Maybe it changes
> timings on your kernel so that it hides the real problem?

I'm not sure this fixed anything or not, it just seemed weird so I changed it.

I was hoping someone that understood rcu locking would comment...

Thanks,
Ben
Johannes Berg Sept. 12, 2016, 6:41 a.m. UTC | #5
> > > -	rcu_read_unlock();
> > >   	spin_unlock_bh(&ar->txqs_lock);
> > > +	rcu_read_unlock();
> > 
> > I'm no RCU expert but this isn't making any sense. Maybe it changes
> > timings on your kernel so that it hides the real problem?
> 
> I'm not sure this fixed anything or not, it just seemed weird so I
> changed it.
> 
> I was hoping someone that understood rcu locking would comment...
> 

RCU is no "locking". The sooner you get over that notion, the better.

This therefore make no sense whatsoever.

In fact, you want to keep the RCU protected section *small*, so having
the spinlock inside hurts overall system performance.

johannes
Ben Greear Sept. 12, 2016, 4:37 p.m. UTC | #6
On 09/11/2016 11:41 PM, Johannes Berg wrote:
>
>>>> -	rcu_read_unlock();
>>>>   	spin_unlock_bh(&ar->txqs_lock);
>>>> +	rcu_read_unlock();
>>>
>>> I'm no RCU expert but this isn't making any sense. Maybe it changes
>>> timings on your kernel so that it hides the real problem?
>>
>> I'm not sure this fixed anything or not, it just seemed weird so I
>> changed it.
>>
>> I was hoping someone that understood rcu locking would comment...
>>
>
> RCU is no "locking". The sooner you get over that notion, the better.
>
> This therefore make no sense whatsoever.
>
> In fact, you want to keep the RCU protected section *small*, so having
> the spinlock inside hurts overall system performance.

Ok, thanks for the review.  I'll drop this patch from my tree.

Thanks,
Ben
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 916119c..d96c06e 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4307,8 +4307,8 @@  void ath10k_mac_tx_push_pending(struct ath10k *ar)
 	int max;
 	int loop_max = 2000;
 
-	spin_lock_bh(&ar->txqs_lock);
 	rcu_read_lock();
+	spin_lock_bh(&ar->txqs_lock);
 
 	last = list_last_entry(&ar->txqs, struct ath10k_txq, list);
 	while (!list_empty(&ar->txqs)) {
@@ -4342,8 +4342,8 @@  void ath10k_mac_tx_push_pending(struct ath10k *ar)
 			break;
 	}
 
-	rcu_read_unlock();
 	spin_unlock_bh(&ar->txqs_lock);
+	rcu_read_unlock();
 }
 
 /************/