Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>>> Kalle Valo  writes:
>>>
 Toke Høiland-Jørgensen  writes:

 I understand your point, but I don't want to rush this to 4.9 and then
 start getting lots of bug reports and eventually forced to revert it. If
 we just found a new serious regression the chances are that there are
 more lurking somewhere and this patch is just not ready yet.
>>>
>>> So, the changes to mac80211 that fixes the known regressions of this
>>> patch have gone in.
>>
>> I guess you mean this commit:
>>
>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ 
>> dequeue
>>
>> (Just making sure that I have the same commit in my tree when I apply
>> this)
>
> Yup, that's the one :)
>
>>> Any chance of seeing this merged during the current merge window? :)
>>
>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>> this has to wait for 4.10.
>
> Ah, right, I think I got my merge windows confused. You already said you
> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
> it into the appropriate -next tree so it can get some wider exposure
> ahead of the *next* merge window...

Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
after I open wireless-drivers-next in 2-3 weeks or so. That would mean
that the patch would hit Linus' tree when 4.10-rc1 is released
(estimated to happen on 2017-01-01). The timing is actually perfect as
now we get maximal testing time on -next.

>> And I assume I need to take v5:
>>
>> https://patchwork.kernel.org/patch/9311037/
>
> Yes. Haven't noticed anything that changed since that might conflict
> with it, but let me know if I missed something and you want a refreshed
> version.

Thanks, I'll let you know if there are any problems.

-- 
Kalle Valo

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> Kalle Valo  writes:
>>
>>> Toke Høiland-Jørgensen  writes:
>>>
>>> I understand your point, but I don't want to rush this to 4.9 and then
>>> start getting lots of bug reports and eventually forced to revert it. If
>>> we just found a new serious regression the chances are that there are
>>> more lurking somewhere and this patch is just not ready yet.
>>
>> So, the changes to mac80211 that fixes the known regressions of this
>> patch have gone in.
>
> I guess you mean this commit:
>
> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
>
> (Just making sure that I have the same commit in my tree when I apply
> this)

Yup, that's the one :)

>> Any chance of seeing this merged during the current merge window? :)
>
> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
> this has to wait for 4.10.

Ah, right, I think I got my merge windows confused. You already said you
wouldn't take it for 4.9. So I guess what I'm asking is for you to put
it into the appropriate -next tree so it can get some wider exposure
ahead of the *next* merge window...

> And I assume I need to take v5:
>
> https://patchwork.kernel.org/patch/9311037/

Yes. Haven't noticed anything that changed since that might conflict
with it, but let me know if I missed something and you want a refreshed
version.

-Toke

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>> I understand your point, but I don't want to rush this to 4.9 and then
>> start getting lots of bug reports and eventually forced to revert it. If
>> we just found a new serious regression the chances are that there are
>> more lurking somewhere and this patch is just not ready yet.
>
> So, the changes to mac80211 that fixes the known regressions of this
> patch have gone in.

I guess you mean this commit:

bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

(Just making sure that I have the same commit in my tree when I apply this)

> Any chance of seeing this merged during the current merge window? :)

I sent last new feature ("-next") patches for 4.9 last week, sorry. So
this has to wait for 4.10.

And I assume I need to take v5:

https://patchwork.kernel.org/patch/9311037/

-- 
Kalle Valo

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

 Well, now that we understand what is causing the throughput regressions,
 fixing them should be fairly straight forward (yeah, famous last words,
 but still...). I already have a patch for the fast path and will go poke
 at the slow path next. It'll probably require another workaround or two,
 so I guess it won't be the architecturally clean ideal solution; but it
 would make it possible to have something that works for 4.9 and then
 iterate for a cleaner design for 4.10.
>>>
>>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>>> are now in -rc3 and let's say that the patches are ready to apply in two
>>> weeks. That would leave us only two weeks of -next time before the merge
>>> window, which I think is not enough for a controversial patch like this
>>> one. There might be other bugs lurking which haven't been found yet.
>>
>> What, other hidden bugs? Unpossible! :)
>
> Yeah, right ;)
>
>> Would it be possible to merge the partial solution (which is ready now,
>> basically) and fix the slow path in a separate patch later?
>
> What do you mean with partial solution? You mean ath9k users would
> suffer from regressions until they are fixed? We can't do that.
>
>> (Just spit-balling here; I'm still fairly new to this process. But I am
>> concerned that we'll hit a catch-22 where we can't get wider testing
>> before it's "ready" and we can't prove that it's "ready" until we've had
>> wider testing...)
>
> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.

So, the changes to mac80211 that fixes the known regressions of this
patch have gone in. Any chance of seeing this merged during the current
merge window? :)

-Toke

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Arend van Spriel]

On 23-08-16 08:59, Kalle Valo wrote:
> Toke Høiland-Jørgensen  writes:
> 
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

 Well, now that we understand what is causing the throughput regressions,
 fixing them should be fairly straight forward (yeah, famous last words,
 but still...). I already have a patch for the fast path and will go poke
 at the slow path next. It'll probably require another workaround or two,
 so I guess it won't be the architecturally clean ideal solution; but it
 would make it possible to have something that works for 4.9 and then
 iterate for a cleaner design for 4.10.
>>>
>>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>>> are now in -rc3 and let's say that the patches are ready to apply in two
>>> weeks. That would leave us only two weeks of -next time before the merge
>>> window, which I think is not enough for a controversial patch like this
>>> one. There might be other bugs lurking which haven't been found yet.
>>
>> What, other hidden bugs? Unpossible! :)
> 
> Yeah, right ;)
> 
>> Would it be possible to merge the partial solution (which is ready now,
>> basically) and fix the slow path in a separate patch later?
> 
> What do you mean with partial solution? You mean ath9k users would
> suffer from regressions until they are fixed? We can't do that.
> 
>> (Just spit-balling here; I'm still fairly new to this process. But I am
>> concerned that we'll hit a catch-22 where we can't get wider testing
>> before it's "ready" and we can't prove that it's "ready" until we've had
>> wider testing...)

So could the wider testing be accomplished by working on a branch in the
wireless-testing repo and make its availability known on wireless-list,
ath?k-list, LWN or whatever.

Regards,
Arend

> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

 This is great work but due to the regressions I'm not sure if this
 will be ready for 4.9. To get more testing time I wonder if we should
 wait for 4.10? IMHO applying this in the end of the cycle is too risky
 and we should try to maximise the time linux-next by applying this
 just after -rc1 is released.

 Thoughts?
>>>
>>> Well, now that we understand what is causing the throughput regressions,
>>> fixing them should be fairly straight forward (yeah, famous last words,
>>> but still...). I already have a patch for the fast path and will go poke
>>> at the slow path next. It'll probably require another workaround or two,
>>> so I guess it won't be the architecturally clean ideal solution; but it
>>> would make it possible to have something that works for 4.9 and then
>>> iterate for a cleaner design for 4.10.
>>
>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>> are now in -rc3 and let's say that the patches are ready to apply in two
>> weeks. That would leave us only two weeks of -next time before the merge
>> window, which I think is not enough for a controversial patch like this
>> one. There might be other bugs lurking which haven't been found yet.
>
> What, other hidden bugs? Unpossible! :)

Yeah, right ;)

> Would it be possible to merge the partial solution (which is ready now,
> basically) and fix the slow path in a separate patch later?

What do you mean with partial solution? You mean ath9k users would
suffer from regressions until they are fixed? We can't do that.

> (Just spit-balling here; I'm still fairly new to this process. But I am
> concerned that we'll hit a catch-22 where we can't get wider testing
> before it's "ready" and we can't prove that it's "ready" until we've had
> wider testing...)

I understand your point, but I don't want to rush this to 4.9 and then
start getting lots of bug reports and eventually forced to revert it. If
we just found a new serious regression the chances are that there are
more lurking somewhere and this patch is just not ready yet.

-- 
Kalle Valo

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> Kalle Valo  writes:
>>
>>> Toke Høiland-Jørgensen  writes:
>>>
 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 v3 (most due to Felix; thanks!):
   - Correctly notify mac80211 when there are packets in the retry queue
 on powersave start/stop.
   - Get rid of ath_tx_aggr_resume().
   - Some readability changes and additional WARN_ON/BUG_ON in
 appropriate places.
>>>
>>> This is great work but due to the regressions I'm not sure if this
>>> will be ready for 4.9. To get more testing time I wonder if we should
>>> wait for 4.10? IMHO applying this in the end of the cycle is too risky
>>> and we should try to maximise the time linux-next by applying this
>>> just after -rc1 is released.
>>>
>>> Thoughts?
>>
>> Well, now that we understand what is causing the throughput regressions,
>> fixing them should be fairly straight forward (yeah, famous last words,
>> but still...). I already have a patch for the fast path and will go poke
>> at the slow path next. It'll probably require another workaround or two,
>> so I guess it won't be the architecturally clean ideal solution; but it
>> would make it possible to have something that works for 4.9 and then
>> iterate for a cleaner design for 4.10.
>
> But if we try to rush this to 4.9 it won't be in linux-next for long. We
> are now in -rc3 and let's say that the patches are ready to apply in two
> weeks. That would leave us only two weeks of -next time before the merge
> window, which I think is not enough for a controversial patch like this
> one. There might be other bugs lurking which haven't been found yet.

What, other hidden bugs? Unpossible! :)

Would it be possible to merge the partial solution (which is ready now,
basically) and fix the slow path in a separate patch later?

(Just spit-balling here; I'm still fairly new to this process. But I am
concerned that we'll hit a catch-22 where we can't get wider testing
before it's "ready" and we can't prove that it's "ready" until we've had
wider testing...)

-Toke

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>>> 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 v3 (most due to Felix; thanks!):
>>>   - Correctly notify mac80211 when there are packets in the retry queue
>>> on powersave start/stop.
>>>   - Get rid of ath_tx_aggr_resume().
>>>   - Some readability changes and additional WARN_ON/BUG_ON in
>>> appropriate places.
>>
>> This is great work but due to the regressions I'm not sure if this
>> will be ready for 4.9. To get more testing time I wonder if we should
>> wait for 4.10? IMHO applying this in the end of the cycle is too risky
>> and we should try to maximise the time linux-next by applying this
>> just after -rc1 is released.
>>
>> Thoughts?
>
> Well, now that we understand what is causing the throughput regressions,
> fixing them should be fairly straight forward (yeah, famous last words,
> but still...). I already have a patch for the fast path and will go poke
> at the slow path next. It'll probably require another workaround or two,
> so I guess it won't be the architecturally clean ideal solution; but it
> would make it possible to have something that works for 4.9 and then
> iterate for a cleaner design for 4.10.

But if we try to rush this to 4.9 it won't be in linux-next for long. We
are now in -rc3 and let's say that the patches are ready to apply in two
weeks. That would leave us only two weeks of -next time before the merge
window, which I think is not enough for a controversial patch like this
one. There might be other bugs lurking which haven't been found yet.

-- 
Kalle Valo

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> 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 v3 (most due to Felix; thanks!):
>>   - Correctly notify mac80211 when there are packets in the retry queue
>> on powersave start/stop.
>>   - Get rid of ath_tx_aggr_resume().
>>   - Some readability changes and additional WARN_ON/BUG_ON in
>> appropriate places.
>
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

Well, now that we understand what is causing the throughput regressions,
fixing them should be fairly straight forward (yeah, famous last words,
but still...). I already have a patch for the fast path and will go poke
at the slow path next. It'll probably require another workaround or two,
so I guess it won't be the architecturally clean ideal solution; but it
would make it possible to have something that works for 4.9 and then
iterate for a cleaner design for 4.10.

-Toke

Re: [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> 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 v3 (most due to Felix; thanks!):
>   - Correctly notify mac80211 when there are packets in the retry queue
> on powersave start/stop.
>   - Get rid of ath_tx_aggr_resume().
>   - Some readability changes and additional WARN_ON/BUG_ON in
> appropriate places.

This is great work but due to the regressions I'm not sure if this will
be ready for 4.9. To get more testing time I wonder if we should wait
for 4.10? IMHO applying this in the end of the cycle is too risky and we
should try to maximise the time linux-next by applying this just after
-rc1 is released.

Thoughts?

-- 
Kalle Valo

[PATCH v4] 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 v3 (most due to Felix; thanks!):
  - Correctly notify mac80211 when there are packets in the retry queue
on powersave start/stop.
  - Get rid of ath_tx_aggr_resume().
  - Some readability changes and additional WARN_ON/BUG_ON in
appropriate places.

 drivers/net/wireless/ath/ath9k/ath9k.h |  27 ++-
 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  |   9 +-
 drivers/net/wireless/ath/ath9k/xmit.c  | 332 +++--
 8 files changed, 157 insertions(+), 235 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h 
b/drivers/net/wireless/ath/ath9k/ath9k.h
index 5294595..7e0a976 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,7 @@ 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_AN_2_TID(_an, _tidno) ath_node_to_tid(_an, _tidno)
 
 #define IS_HT_RATE(rate)   (rate & 0x80)
 #define IS_CCK_RATE(rate)  ((rate >= 0x18) && (rate <= 0x1e))
@@ -164,7 +163,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 +230,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 +244,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 +273,6 @@ struct ath_tx_control {
struct ath_node *an;
struct ieee80211_sta *sta;
u8 paprd;
-   bool force_channel;
 };
 
 
@@ -293,7 +289,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];
 };
 
@@ -421,6 +416,22 @@ struct ath_offchannel {
int duration;
 };
 
+static inline struct ath_atx_tid *
+ath_node_to_tid(struct ath_node *an, u8 tidno)
+{
+   struct ieee80211_sta *sta = an->sta;
+   struct ieee80211_vif *vif = an->vif;
+   struct ieee80211_txq *txq;
+
+   BUG_ON(!vif);
+   if (sta)
+   txq = sta->txq[tidno % ARRAY_SIZE(sta->txq)];
+   else
+   txq = vif->txq;
+
+   return (struct ath_atx_tid *) txq->drv_priv;
+}
+
 #define case_rtn_string(val) case val: return #val
 
 #define ath_for_each_chanctx(_sc, _ctx) \
@@ -575,7 +586,6 @@ void ath_tx_edma_tasklet(struct ath_softc *sc);
 int ath_tx_aggr_start(struct ath_softc *sc, struct ieee80211_sta *sta,
  u16 tid, u16 *ssn);
 void ath_tx_aggr_stop(struct ath_softc *sc, struct ieee80211_sta *sta, u16 
tid);
-void ath_tx_aggr_resume(struct ath_softc *sc, struct ieee80211_sta *sta, u16 
tid);
 
 void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an);
 void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
@@ -585,6 +595,7 @@ void ath9k_release_buffered_frames(struct ieee80211_hw *hw,