Re: [ath9k-devel] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Felix Fietkau  writes:

> On 2016-07-06 20:52, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau  writes:
>> 
>>> On 2016-07-06 18:16, Toke Høiland-Jørgensen wrote:
 This switches ath9k over to using the mac80211 intermediate software
 queueing mechanism for data packets. It removes the queueing inside the
 driver, except for the retry queue, and instead pulls from mac80211 when
 a packet is needed. The retry queue is used to store a packet that was
 pulled but can't be sent immediately.
 
 The old code path in ath_tx_start that would queue packets has been
 removed completely, as has the qlen limit tunables (since there's no
 longer a queue in the driver to limit).
 
 Based on Tim's original patch set, but reworked quite thoroughly.
 
 Cc: Tim Shepard 
 Cc: Felix Fietkau 
 Signed-off-by: Toke Høiland-Jørgensen 
 ---
 Changes since v1:
   - Remove the old intermediate queueing logic completely instead of
 just disabling it.
   - Remove the qlen debug tunables.
   - Remove the force_channel parameter from struct txctl (since we just
 removed the code path that was using it).
 
  drivers/net/wireless/ath/ath9k/ath9k.h |  12 +-
  drivers/net/wireless/ath/ath9k/channel.c   |   2 -
  drivers/net/wireless/ath/ath9k/debug.c |  14 +-
  drivers/net/wireless/ath/ath9k/debug.h |   2 -
  drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
  drivers/net/wireless/ath/ath9k/init.c  |   2 +-
  drivers/net/wireless/ath/ath9k/main.c  |   1 +
  drivers/net/wireless/ath/ath9k/xmit.c  | 307 
 +++--
  8 files changed, 130 insertions(+), 214 deletions(-)
>>> Nice work!
>> 
>> Thanks :)
>> 
 diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
 b/drivers/net/wireless/ath/ath9k/xmit.c
 index fe795fc..4077eeb 100644
 --- a/drivers/net/wireless/ath/ath9k/xmit.c
 +++ b/drivers/net/wireless/ath/ath9k/xmit.c
 @@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct 
 ath_txq *txq,
seqno = bf->bf_state.seqno;
  
/* do not step over block-ack window */
 -  if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
 +  if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
 +  __skb_queue_tail(>retry_q, skb);
 +
 +  /* If there are other skbs in the retry q, they are
 +   * probably within the BAW, so loop immediately to get
 +   * one of them. Otherwise the queue can get stuck. */
 +  if (!skb_queue_is_first(>retry_q, skb))
 +  continue;
>>> Not sure if this can happen, but if we ever somehow end up with two skbs
>>> in the retry queue that do not fit into the Block-Ack window, there's
>>> potential for an infinite loop here.
>> 
>> Yes, I realise that (v1 contained a comment on that). However, I don't
>> actually think it can happen: The code will only ever put one skb from
>> the intermediate queues on the retry queue (ath_tid_pull() is only
>> called if the retry queue is empty). Everything else on there are actual
>> retries that have been put on there by ath_tx_complete_aggr(). Figure
>> the latter will always be within the BAW?
> I think it would be a good idea to have a check there (with WARN_ON), in
> case there's some weird corner case with seqno handling, software retry
> and aggregation state changes.

Right, can do :)

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

[ath9k-devel] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

This switches ath9k over to using the mac80211 intermediate software
queueing mechanism for data packets. It removes the queueing inside the
driver, except for the retry queue, and instead pulls from mac80211 when
a packet is needed. The retry queue is used to store a packet that was
pulled but can't be sent immediately.

The old code path in ath_tx_start that would queue packets has been
removed completely, as has the qlen limit tunables (since there's no
longer a queue in the driver to limit).

Based on Tim's original patch set, but reworked quite thoroughly.

Cc: Tim Shepard 
Cc: Felix Fietkau 
Signed-off-by: Toke Høiland-Jørgensen 
---
Changes since v1:
  - Remove the old intermediate queueing logic completely instead of
just disabling it.
  - Remove the qlen debug tunables.
  - Remove the force_channel parameter from struct txctl (since we just
removed the code path that was using it).

 drivers/net/wireless/ath/ath9k/ath9k.h |  12 +-
 drivers/net/wireless/ath/ath9k/channel.c   |   2 -
 drivers/net/wireless/ath/ath9k/debug.c |  14 +-
 drivers/net/wireless/ath/ath9k/debug.h |   2 -
 drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
 drivers/net/wireless/ath/ath9k/init.c  |   2 +-
 drivers/net/wireless/ath/ath9k/main.c  |   1 +
 drivers/net/wireless/ath/ath9k/xmit.c  | 307 +++--
 8 files changed, 130 insertions(+), 214 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 5294595..daf972c 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -91,7 +91,6 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define ATH_RXBUF   512
 #define ATH_TXBUF   512
 #define ATH_TXBUF_RESERVE   5
-#define ATH_MAX_QDEPTH  (ATH_TXBUF / 4 - ATH_TXBUF_RESERVE)
 #define ATH_TXMAXTRY13
 #define ATH_MAX_SW_RETRIES  30
 
@@ -145,7 +144,9 @@ int ath_descdma_setup(struct ath_softc *sc, struct 
ath_descdma *dd,
 #define BAW_WITHIN(_start, _bawsz, _seqno) \
_seqno) - (_start)) & 4095) < (_bawsz))
 
-#define ATH_AN_2_TID(_an, _tidno)  (&(_an)->tid[(_tidno)])
+#define ATH_STA_2_TID(_sta, _tidno) ((struct ath_atx_tid 
*)(_sta)->txq[_tidno]->drv_priv)
+#define ATH_VIF_2_TID(_vif) ((struct ath_atx_tid *)(_vif)->txq->drv_priv)
+#define ATH_AN_2_TID(_an, _tidno) ((_an)->sta ? ATH_STA_2_TID((_an)->sta, 
_tidno) : ATH_VIF_2_TID((_an)->vif))
 
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
@@ -164,7 +165,6 @@ struct ath_txq {
spinlock_t axq_lock;
u32 axq_depth;
u32 axq_ampdu_depth;
-   bool stopped;
bool axq_tx_inprogress;
struct list_head txq_fifo[ATH_TXFIFO_DEPTH];
u8 txq_headidx;
@@ -232,7 +232,6 @@ struct ath_buf {
 
 struct ath_atx_tid {
struct list_head list;
-   struct sk_buff_head buf_q;
struct sk_buff_head retry_q;
struct ath_node *an;
struct ath_txq *txq;
@@ -247,13 +246,13 @@ struct ath_atx_tid {
s8 bar_index;
bool active;
bool clear_ps_filter;
+   bool has_queued;
 };
 
 struct ath_node {
struct ath_softc *sc;
struct ieee80211_sta *sta; /* station struct we're part of */
struct ieee80211_vif *vif; /* interface with which we're associated */
-   struct ath_atx_tid tid[IEEE80211_NUM_TIDS];
 
u16 maxampdu;
u8 mpdudensity;
@@ -276,7 +275,6 @@ struct ath_tx_control {
struct ath_node *an;
struct ieee80211_sta *sta;
u8 paprd;
-   bool force_channel;
 };
 
 
@@ -293,7 +291,6 @@ struct ath_tx {
struct ath_descdma txdma;
struct ath_txq *txq_map[IEEE80211_NUM_ACS];
struct ath_txq *uapsdq;
-   u32 txq_max_pending[IEEE80211_NUM_ACS];
u16 max_aggr_framelen[IEEE80211_NUM_ACS][4][32];
 };
 
@@ -585,6 +582,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,
   u16 tids, int nframes,
   enum ieee80211_frame_release_type reason,
   bool more_data);
+void ath9k_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *queue);
 
 //
 /* VIFs */
diff --git a/drivers/net/wireless/ath/ath9k/channel.c 
b/drivers/net/wireless/ath/ath9k/channel.c
index 319cb5f..a5ce016 100644
--- a/drivers/net/wireless/ath/ath9k/channel.c
+++ b/drivers/net/wireless/ath/ath9k/channel.c
@@ -1007,7 +1007,6 @@ static void ath_scan_send_probe(struct ath_softc *sc,
goto error;
 
txctl.txq = sc->tx.txq_map[IEEE80211_AC_VO];
-   txctl.force_channel = true;
if (ath_tx_start(sc->hw, skb, ))
goto error;
 
@@ -1130,7 +1129,6 @@ ath_chanctx_send_vif_ps_frame(struct ath_softc *sc, 
struct ath_vif *avp,
memset(, 0, sizeof(txctl));

Re: [ath9k-devel] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Felix Fietkau  writes:

> On 2016-07-06 18:16, Toke Høiland-Jørgensen wrote:
>> This switches ath9k over to using the mac80211 intermediate software
>> queueing mechanism for data packets. It removes the queueing inside the
>> driver, except for the retry queue, and instead pulls from mac80211 when
>> a packet is needed. The retry queue is used to store a packet that was
>> pulled but can't be sent immediately.
>> 
>> The old code path in ath_tx_start that would queue packets has been
>> removed completely, as has the qlen limit tunables (since there's no
>> longer a queue in the driver to limit).
>> 
>> Based on Tim's original patch set, but reworked quite thoroughly.
>> 
>> Cc: Tim Shepard 
>> Cc: Felix Fietkau 
>> Signed-off-by: Toke Høiland-Jørgensen 
>> ---
>> Changes since v1:
>>   - Remove the old intermediate queueing logic completely instead of
>> just disabling it.
>>   - Remove the qlen debug tunables.
>>   - Remove the force_channel parameter from struct txctl (since we just
>> removed the code path that was using it).
>> 
>>  drivers/net/wireless/ath/ath9k/ath9k.h |  12 +-
>>  drivers/net/wireless/ath/ath9k/channel.c   |   2 -
>>  drivers/net/wireless/ath/ath9k/debug.c |  14 +-
>>  drivers/net/wireless/ath/ath9k/debug.h |   2 -
>>  drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
>>  drivers/net/wireless/ath/ath9k/init.c  |   2 +-
>>  drivers/net/wireless/ath/ath9k/main.c  |   1 +
>>  drivers/net/wireless/ath/ath9k/xmit.c  | 307 
>> +++--
>>  8 files changed, 130 insertions(+), 214 deletions(-)
> Nice work!

Thanks :)

>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
>> b/drivers/net/wireless/ath/ath9k/xmit.c
>> index fe795fc..4077eeb 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct 
>> ath_txq *txq,
>>  seqno = bf->bf_state.seqno;
>>  
>>  /* do not step over block-ack window */
>> -if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
>> +if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
>> +__skb_queue_tail(>retry_q, skb);
>> +
>> +/* If there are other skbs in the retry q, they are
>> + * probably within the BAW, so loop immediately to get
>> + * one of them. Otherwise the queue can get stuck. */
>> +if (!skb_queue_is_first(>retry_q, skb))
>> +continue;
> Not sure if this can happen, but if we ever somehow end up with two skbs
> in the retry queue that do not fit into the Block-Ack window, there's
> potential for an infinite loop here.

Yes, I realise that (v1 contained a comment on that). However, I don't
actually think it can happen: The code will only ever put one skb from
the intermediate queues on the retry queue (ath_tid_pull() is only
called if the retry queue is empty). Everything else on there are actual
retries that have been put on there by ath_tx_complete_aggr(). Figure
the latter will always be within the BAW?

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues.

[Felix Fietkau]

On 2016-07-06 20:52, Toke Høiland-Jørgensen wrote:
> Felix Fietkau  writes:
> 
>> On 2016-07-06 18:16, Toke Høiland-Jørgensen wrote:
>>> This switches ath9k over to using the mac80211 intermediate software
>>> queueing mechanism for data packets. It removes the queueing inside the
>>> driver, except for the retry queue, and instead pulls from mac80211 when
>>> a packet is needed. The retry queue is used to store a packet that was
>>> pulled but can't be sent immediately.
>>> 
>>> The old code path in ath_tx_start that would queue packets has been
>>> removed completely, as has the qlen limit tunables (since there's no
>>> longer a queue in the driver to limit).
>>> 
>>> Based on Tim's original patch set, but reworked quite thoroughly.
>>> 
>>> Cc: Tim Shepard 
>>> Cc: Felix Fietkau 
>>> Signed-off-by: Toke Høiland-Jørgensen 
>>> ---
>>> Changes since v1:
>>>   - Remove the old intermediate queueing logic completely instead of
>>> just disabling it.
>>>   - Remove the qlen debug tunables.
>>>   - Remove the force_channel parameter from struct txctl (since we just
>>> removed the code path that was using it).
>>> 
>>>  drivers/net/wireless/ath/ath9k/ath9k.h |  12 +-
>>>  drivers/net/wireless/ath/ath9k/channel.c   |   2 -
>>>  drivers/net/wireless/ath/ath9k/debug.c |  14 +-
>>>  drivers/net/wireless/ath/ath9k/debug.h |   2 -
>>>  drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
>>>  drivers/net/wireless/ath/ath9k/init.c  |   2 +-
>>>  drivers/net/wireless/ath/ath9k/main.c  |   1 +
>>>  drivers/net/wireless/ath/ath9k/xmit.c  | 307 
>>> +++--
>>>  8 files changed, 130 insertions(+), 214 deletions(-)
>> Nice work!
> 
> Thanks :)
> 
>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
>>> b/drivers/net/wireless/ath/ath9k/xmit.c
>>> index fe795fc..4077eeb 100644
>>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>> @@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct 
>>> ath_txq *txq,
>>> seqno = bf->bf_state.seqno;
>>>  
>>> /* do not step over block-ack window */
>>> -   if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
>>> +   if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
>>> +   __skb_queue_tail(>retry_q, skb);
>>> +
>>> +   /* If there are other skbs in the retry q, they are
>>> +* probably within the BAW, so loop immediately to get
>>> +* one of them. Otherwise the queue can get stuck. */
>>> +   if (!skb_queue_is_first(>retry_q, skb))
>>> +   continue;
>> Not sure if this can happen, but if we ever somehow end up with two skbs
>> in the retry queue that do not fit into the Block-Ack window, there's
>> potential for an infinite loop here.
> 
> Yes, I realise that (v1 contained a comment on that). However, I don't
> actually think it can happen: The code will only ever put one skb from
> the intermediate queues on the retry queue (ath_tid_pull() is only
> called if the retry queue is empty). Everything else on there are actual
> retries that have been put on there by ath_tx_complete_aggr(). Figure
> the latter will always be within the BAW?
I think it would be a good idea to have a check there (with WARN_ON), in
case there's some weird corner case with seqno handling, software retry
and aggregation state changes.

- Felix
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues.

[Felix Fietkau]

On 2016-07-06 18:16, Toke Høiland-Jørgensen wrote:
> This switches ath9k over to using the mac80211 intermediate software
> queueing mechanism for data packets. It removes the queueing inside the
> driver, except for the retry queue, and instead pulls from mac80211 when
> a packet is needed. The retry queue is used to store a packet that was
> pulled but can't be sent immediately.
> 
> The old code path in ath_tx_start that would queue packets has been
> removed completely, as has the qlen limit tunables (since there's no
> longer a queue in the driver to limit).
> 
> Based on Tim's original patch set, but reworked quite thoroughly.
> 
> Cc: Tim Shepard 
> Cc: Felix Fietkau 
> Signed-off-by: Toke Høiland-Jørgensen 
> ---
> Changes since v1:
>   - Remove the old intermediate queueing logic completely instead of
> just disabling it.
>   - Remove the qlen debug tunables.
>   - Remove the force_channel parameter from struct txctl (since we just
> removed the code path that was using it).
> 
>  drivers/net/wireless/ath/ath9k/ath9k.h |  12 +-
>  drivers/net/wireless/ath/ath9k/channel.c   |   2 -
>  drivers/net/wireless/ath/ath9k/debug.c |  14 +-
>  drivers/net/wireless/ath/ath9k/debug.h |   2 -
>  drivers/net/wireless/ath/ath9k/debug_sta.c |   4 +-
>  drivers/net/wireless/ath/ath9k/init.c  |   2 +-
>  drivers/net/wireless/ath/ath9k/main.c  |   1 +
>  drivers/net/wireless/ath/ath9k/xmit.c  | 307 
> +++--
>  8 files changed, 130 insertions(+), 214 deletions(-)
Nice work!


> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
> b/drivers/net/wireless/ath/ath9k/xmit.c
> index fe795fc..4077eeb 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -912,8 +923,16 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct 
> ath_txq *txq,
>   seqno = bf->bf_state.seqno;
>  
>   /* do not step over block-ack window */
> - if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno))
> + if (!BAW_WITHIN(tid->seq_start, tid->baw_size, seqno)) {
> + __skb_queue_tail(>retry_q, skb);
> +
> + /* If there are other skbs in the retry q, they are
> +  * probably within the BAW, so loop immediately to get
> +  * one of them. Otherwise the queue can get stuck. */
> + if (!skb_queue_is_first(>retry_q, skb))
> + continue;
Not sure if this can happen, but if we ever somehow end up with two skbs
in the retry queue that do not fit into the Block-Ack window, there's
potential for an infinite loop here.

- Felix
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel