[Make-wifi-fast] Fwd: [PATCH] ath10k: Restart xmit queues below low-water mark.

[Dave Taht]

thoughts?

---------- Forwarded message ---------
From: Ben Greear <greearb at candelatech.com>
Date: Tue, Apr 28, 2020 at 7:58 AM
Subject: Re: [PATCH] ath10k: Restart xmit queues below low-water mark.
To: Steve deRosier <derosier at gmail.com>, <dave.taht at gmail.com>
Cc: linux-wireless <linux-wireless at vger.kernel.org>




On 04/28/2020 07:56 AM, Steve deRosier wrote:
> On Mon, Apr 27, 2020 at 7:54 AM <greearb at candelatech.com> wrote:
>>
>> From: Ben Greear <greearb at candelatech.com>
>>
>> While running tcp upload + download tests with ~200
>> concurrent TCP streams, 1-2 processes, and 30 station
>> vdevs, I noticed that the __ieee80211_stop_queue was taking
>> around 20% of the CPU according to perf-top, which other locking
>> taking an additional ~15%.
>>
>> I believe the issue is that the ath10k driver would unlock the
>> txqueue when a single frame could be transmitted, instead of
>> waiting for a low water mark.
>>
>> So, this patch adds a low-water mark that is 1/4 of the total
>> tx buffers allowed.
>>
>> This appears to resolve the performance problem that I saw.
>>
>> Tested with recent wave-1 ath10k-ct firmware.
>>
>
> Hey Ben,
>
> Did you do any testing with this patch around latency?  The nature of
> the thing that you fixed makes me wonder if it was intentional with
> respect to making WiFi fast - ie getting rid of buffers as much as
> possible.  Obviously the CPU impact is likely to be an unintended
> consequence. In any case, I don't know anything for sure, it was just
> a thought that went through my head when reading this.

I did not, but on average my patch should make the queues be less full,
so I doubt it will hurt latency.

Thanks,
Ben

>
>
>> Signed-off-by: Ben Greear <greearb at candelatech.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/htt.h    | 1 +
>>  drivers/net/wireless/ath/ath10k/htt_tx.c | 8 ++++++--
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
>> index 31c4ddbf45cb..b5634781c0dc 100644
>> --- a/drivers/net/wireless/ath/ath10k/htt.h
>> +++ b/drivers/net/wireless/ath/ath10k/htt.h
>> @@ -1941,6 +1941,7 @@ struct ath10k_htt {
>>
>>         u8 target_version_major;
>>         u8 target_version_minor;
>> +       bool needs_unlock;
>>         struct completion target_version_received;
>>         u8 max_num_amsdu;
>>         u8 max_num_ampdu;
>> diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
>> index 9b3c3b080e92..44795d9a7c0c 100644
>> --- a/drivers/net/wireless/ath/ath10k/htt_tx.c
>> +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
>> @@ -145,8 +145,10 @@ void ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
>>         lockdep_assert_held(&htt->tx_lock);
>>
>>         htt->num_pending_tx--;
>> -       if (htt->num_pending_tx == htt->max_num_pending_tx - 1)
>> +       if ((htt->num_pending_tx <= (htt->max_num_pending_tx / 4)) && htt->needs_unlock) {
>> +               htt->needs_unlock = false;
>>                 ath10k_mac_tx_unlock(htt->ar, ATH10K_TX_PAUSE_Q_FULL);
>> +       }
>>  }
>>
>>  int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
>> @@ -157,8 +159,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
>>                 return -EBUSY;
>>
>>         htt->num_pending_tx++;
>> -       if (htt->num_pending_tx == htt->max_num_pending_tx)
>> +       if (htt->num_pending_tx == htt->max_num_pending_tx) {
>> +               htt->needs_unlock = true;
>>                 ath10k_mac_tx_lock(htt->ar, ATH10K_TX_PAUSE_Q_FULL);
>> +       }
>>
>>         return 0;
>>  }
>> --
>> 2.20.1
>>
>

--
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


-- 
Make Music, Not War

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-435-0729