diff mbox series

[1/4] ath10k: improve tx status reporting

Message ID 20220516032519.29831-2-ryazanov.s.a@gmail.com
State New
Headers show
Series ath10k: add encapsulation offloading support | expand

Commit Message

Sergey Ryazanov May 16, 2022, 3:25 a.m. UTC 
We use ieee80211_tx_status() to report each completed tx frame.
Internally, this function calls sta_info_get_by_addrs(), what has a
couple of drawbacks:
1. additional station lookup causes a performance degradation;
2. mac80211 can not properly account Ethernet encapsulated frames due
   to the inability to properly determine the destination (station) MAC
   address since ieee80211_tx_status() assumes the frame has a 802.11
   header.

The latter is especially destructive if we want to use hardware frames
encapsulation.

To fix both of these issues, replace ieee80211_tx_status() with
ieee80211_tx_status_ext() call and feed it station pointer from the tx
queue associated with the transmitted frame.

Tested-on: QCA9888 hw 2.0 10.4-3.9.0.2-00131
Tested-on: QCA6174 hw 3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Tested-by: Oldřich Jedlička <oldium.pro@gmail.com> # TP-Link Archer C7 v4 & v5 (QCA9563 + QCA9880)
Tested-by: Edward Matijevic <motolav@gmail.com> # TP-Link Archer C2600 (IPQ8064 + QCA9980 10.4.1.00030-1)
Tested-by: Edward Matijevic <motolav@gmail.com> # QCA9377 PCI in Sta mode
Tested-by: Zhijun You <hujy652@gmail.com> # NETGEAR R7800 (QCA9984 10.4-3.9.0.2-00159)
---

Changes since RFC:
 * new Tested-on and Tested-by tags

 drivers/net/wireless/ath/ath10k/txrx.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Sergey Ryazanov May 16, 2022, 8:57 p.m. UTC | #1 
On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> We use ieee80211_tx_status() to report each completed tx frame.
> Internally, this function calls sta_info_get_by_addrs(), what has a
> couple of drawbacks:
> 1. additional station lookup causes a performance degradation;
> 2. mac80211 can not properly account Ethernet encapsulated frames due
>    to the inability to properly determine the destination (station) MAC
>    address since ieee80211_tx_status() assumes the frame has a 802.11
>    header.
>
> The latter is especially destructive if we want to use hardware frames
> encapsulation.
>
> To fix both of these issues, replace ieee80211_tx_status() with
> ieee80211_tx_status_ext() call and feed it station pointer from the tx
> queue associated with the transmitted frame.
>
> Tested-on: QCA9888 hw 2.0 10.4-3.9.0.2-00131
> Tested-on: QCA6174 hw 3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Tested-by: Oldřich Jedlička <oldium.pro@gmail.com> # TP-Link Archer C7 v4 & v5 (QCA9563 + QCA9880)
> Tested-by: Edward Matijevic <motolav@gmail.com> # TP-Link Archer C2600 (IPQ8064 + QCA9980 10.4.1.00030-1)
> Tested-by: Edward Matijevic <motolav@gmail.com> # QCA9377 PCI in Sta mode
> Tested-by: Zhijun You <hujy652@gmail.com> # NETGEAR R7800 (QCA9984 10.4-3.9.0.2-00159)
> ---
>
> Changes since RFC:
>  * new Tested-on and Tested-by tags
>
>  drivers/net/wireless/ath/ath10k/txrx.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
> index 10123974c3da..72540434c75b 100644
> --- a/drivers/net/wireless/ath/ath10k/txrx.c
> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb)
>  int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>                          const struct htt_tx_done *tx_done)
>  {
> +       struct ieee80211_tx_status status;
>         struct ath10k *ar = htt->ar;
>         struct device *dev = ar->dev;
>         struct ieee80211_tx_info *info;
> @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>                 info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
>         }
>
> -       ieee80211_tx_status(htt->ar->hw, msdu);
> +       memset(&status, 0, sizeof(status));
> +       status.skb = msdu;
> +       status.info = info;
> +
> +       rcu_read_lock();
> +       if (txq && txq->sta)
> +               status.sta = txq->sta;

Just noticed that since we do not dereference the txq->sta pointer
here, the above code can be simplified to:

if (txq)
        status.sta = txq->sta;

Kalle, should I send V2 or can you change this in your tree?

> +       ieee80211_tx_status_ext(htt->ar->hw, &status);
> +       rcu_read_unlock();
> +
>         /* we do not own the msdu anymore */
>
>         return 0;
Kalle Valo May 18, 2022, 7:30 a.m. UTC | #2 
Sergey Ryazanov <ryazanov.s.a@gmail.com> writes:
> On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
>> --- a/drivers/net/wireless/ath/ath10k/txrx.c
>> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
>> @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb)
>>  int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>                          const struct htt_tx_done *tx_done)
>>  {
>> +       struct ieee80211_tx_status status;
>>         struct ath10k *ar = htt->ar;
>>         struct device *dev = ar->dev;
>>         struct ieee80211_tx_info *info;
>> @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>                 info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
>>         }
>>
>> -       ieee80211_tx_status(htt->ar->hw, msdu);
>> +       memset(&status, 0, sizeof(status));
>> +       status.skb = msdu;
>> +       status.info = info;
>> +
>> +       rcu_read_lock();
>> +       if (txq && txq->sta)
>> +               status.sta = txq->sta;
>
> Just noticed that since we do not dereference the txq->sta pointer
> here, the above code can be simplified to:
>
> if (txq)
>         status.sta = txq->sta;
>
> Kalle, should I send V2 or can you change this in your tree?

I changed this in the pending branch, please check my changes:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=1bd0c16e10229683fab1dd8adf8c4339992688b7
Sergey Ryazanov May 18, 2022, 10:57 a.m. UTC | #3 
On Wed, May 18, 2022 at 10:30 AM Kalle Valo <kvalo@kernel.org> wrote:
> Sergey Ryazanov <ryazanov.s.a@gmail.com> writes:
>> On Mon, May 16, 2022 at 6:25 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>>> --- a/drivers/net/wireless/ath/ath10k/txrx.c
>>> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
>>> @@ -43,6 +43,7 @@ static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb)
>>>  int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>                          const struct htt_tx_done *tx_done)
>>>  {
>>> +       struct ieee80211_tx_status status;
>>>         struct ath10k *ar = htt->ar;
>>>         struct device *dev = ar->dev;
>>>         struct ieee80211_tx_info *info;
>>> @@ -128,7 +129,16 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>>                 info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
>>>         }
>>>
>>> -       ieee80211_tx_status(htt->ar->hw, msdu);
>>> +       memset(&status, 0, sizeof(status));
>>> +       status.skb = msdu;
>>> +       status.info = info;
>>> +
>>> +       rcu_read_lock();
>>> +       if (txq && txq->sta)
>>> +               status.sta = txq->sta;
>>
>> Just noticed that since we do not dereference the txq->sta pointer
>> here, the above code can be simplified to:
>>
>> if (txq)
>>         status.sta = txq->sta;
>>
>> Kalle, should I send V2 or can you change this in your tree?
>
> I changed this in the pending branch, please check my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=1bd0c16e10229683fab1dd8adf8c4339992688b7

Exactly what I meant. Thank you!

--
Sergey
Kalle Valo May 22, 2022, 12:28 p.m. UTC | #4 
Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:

> We use ieee80211_tx_status() to report each completed tx frame.
> Internally, this function calls sta_info_get_by_addrs(), what has a
> couple of drawbacks:
> 1. additional station lookup causes a performance degradation;
> 2. mac80211 can not properly account Ethernet encapsulated frames due
>    to the inability to properly determine the destination (station) MAC
>    address since ieee80211_tx_status() assumes the frame has a 802.11
>    header.
> 
> The latter is especially destructive if we want to use hardware frames
> encapsulation.
> 
> To fix both of these issues, replace ieee80211_tx_status() with
> ieee80211_tx_status_ext() call and feed it station pointer from the tx
> queue associated with the transmitted frame.
> 
> Tested-on: QCA9888 hw2.0 PCI 10.4-3.9.0.2-00131
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> 
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Tested-by: Oldřich Jedlička <oldium.pro@gmail.com> # TP-Link Archer C7 v4 & v5 (QCA9563 + QCA9880)
> Tested-by: Edward Matijevic <motolav@gmail.com> # TP-Link Archer C2600 (IPQ8064 + QCA9980 10.4.1.00030-1)
> Tested-by: Edward Matijevic <motolav@gmail.com> # QCA9377 PCI in Sta mode
> Tested-by: Zhijun You <hujy652@gmail.com> # NETGEAR R7800 (QCA9984 10.4-3.9.0.2-00159)
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

4 patches applied to ath-next branch of ath.git, thanks.

2587d5198aa5 ath10k: improve tx status reporting
70f119fb82af ath10k: htt_tx: do not interpret Eth frames as WiFi
a09740548275 ath10k: turn rawmode into frame_mode
af6d8265c47e ath10k: add encapsulation offloading support
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 10123974c3da..72540434c75b 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -43,6 +43,7 @@  static void ath10k_report_offchan_tx(struct ath10k *ar, struct sk_buff *skb)
 int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
 			 const struct htt_tx_done *tx_done)
 {
+	struct ieee80211_tx_status status;
 	struct ath10k *ar = htt->ar;
 	struct device *dev = ar->dev;
 	struct ieee80211_tx_info *info;
@@ -128,7 +129,16 @@  int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
 		info->status.flags |= IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
 	}
 
-	ieee80211_tx_status(htt->ar->hw, msdu);
+	memset(&status, 0, sizeof(status));
+	status.skb = msdu;
+	status.info = info;
+
+	rcu_read_lock();
+	if (txq && txq->sta)
+		status.sta = txq->sta;
+	ieee80211_tx_status_ext(htt->ar->hw, &status);
+	rcu_read_unlock();
+
 	/* we do not own the msdu anymore */
 
 	return 0;