From: "Toke Høiland-Jørgensen" <toke@toke.dk> To: Tim Shepard <shep@alum.mit.edu> Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, ath9k-devel@lists.ath9k.org, Felix Fietkau <nbd@nbd.name> Subject: Re: [PATCH] ath9k: Switch to using mac80211 intermediate software queues. Date: Sun, 19 Jun 2016 15:50:23 +0200 [thread overview] Message-ID: <87twgpqodc.fsf@toke.dk> (raw) In-Reply-To: <E1bEcwS-0006jR-00@www.xplot.org> (Tim Shepard's message of "Sun, 19 Jun 2016 09:39:12 -0400") Tim Shepard <shep@alum.mit.edu> writes: >> >> You're right that it doesn't check the max. However, this is less of a >> problem now that there is no intermediate queueing in the driver; and >> indeed the utility of haven the qlen_* tunables is somewhat questionable >> with the patch applied: The only thing this is going to control is the >> size of the retry queue, and possible limit the size of the retry queue. >> [....] > > The driver queues things up for the hardware to DMA and transmit. > Something has to limit the amount of packets handed over to the > hardware. (We lack access to hardware documentation (grrrr!) but it > appears to me that the hardware has a hard limit on how many packets > can be handed to it.) There's a ring buffer eight entries long that the aggregates (or packets) are put on when actually being handed to the hardware. This is in ath_txq->txq_fifo. >> Because there's a second limit in play (which has always been there): in >> ath_tx_sched_aggr() there is this check: >> >> if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) || >> (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) { >> __skb_queue_tail(&tid->retry_q, bf->bf_mpdu); >> *stop = true; >> return false; >> } >> >> The two constants are 2 and 8 respectively. This means that, with >> aggregation enabled, no more than two full aggregates will be queued up. >> The size of the aggregates is dynamically computed from the current >> rate: they are limited a maximum of four milliseconds of (estimated) >> airtime (for the BE queue; the others have different limits). >> >> So in a sense there's already a dynamic limit on the hardware queues. >> Now, whether four milliseconds is the right maximum aggregate size might >> be worth discussing. It is the maximum allowed by the standard. Dave and >> I have been > > Ah that may be the clue that I lacked. There's got to be a dependency > on processor speed (how quickly the system and driver can get another > packet hooked up for transmission after completions) but perhaps with > aggregates being so large in time, with full aggregates even the > slowest processors are fast enough to avoid starvation. > > If there's no aggregation, a limit of some sort is needed (probably to > prevent malfunction of the hardware/driver, but in any case to limit > excess latency). And this limit will depend on processor speed (and > will need to be autotuned at runtime). ATH_NON_AGGR_MIN_QDEPTH is 8 -- so yeah, the limit is higher if there is no aggregation. These are hard-coded values, so presumably they are large enough to keep the hardware busy on most platforms (or someone would have noticed and changed them?). So I doubt there is much to be gained to add a mechanism to dynamically tune them (between 0 and 2?). The exception being in case pulling from the mac80211 queue is too slow to keep the hardware busy at the current settings. I see no problems with this on my hardware, but that's an x86 box. I would probably hold off on the dynamic tuning until having proven that there's actually a bottleneck, though... ;) -Toke
WARNING: multiple messages have this Message-ID (diff)
From: "Toke Høiland-Jørgensen" <toke@toke.dk> To: ath9k-devel@lists.ath9k.org Subject: [ath9k-devel] [PATCH] ath9k: Switch to using mac80211 intermediate software queues. Date: Sun, 19 Jun 2016 13:50:31 -0000 [thread overview] Message-ID: <87twgpqodc.fsf@toke.dk> (raw) In-Reply-To: <E1bEcwS-0006jR-00@www.xplot.org> (Tim Shepard's message of "Sun, 19 Jun 2016 09:39:12 -0400") Tim Shepard <shep@alum.mit.edu> writes: >> >> You're right that it doesn't check the max. However, this is less of a >> problem now that there is no intermediate queueing in the driver; and >> indeed the utility of haven the qlen_* tunables is somewhat questionable >> with the patch applied: The only thing this is going to control is the >> size of the retry queue, and possible limit the size of the retry queue. >> [....] > > The driver queues things up for the hardware to DMA and transmit. > Something has to limit the amount of packets handed over to the > hardware. (We lack access to hardware documentation (grrrr!) but it > appears to me that the hardware has a hard limit on how many packets > can be handed to it.) There's a ring buffer eight entries long that the aggregates (or packets) are put on when actually being handed to the hardware. This is in ath_txq->txq_fifo. >> Because there's a second limit in play (which has always been there): in >> ath_tx_sched_aggr() there is this check: >> >> if ((aggr && txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH) || >> (!aggr && txq->axq_depth >= ATH_NON_AGGR_MIN_QDEPTH)) { >> __skb_queue_tail(&tid->retry_q, bf->bf_mpdu); >> *stop = true; >> return false; >> } >> >> The two constants are 2 and 8 respectively. This means that, with >> aggregation enabled, no more than two full aggregates will be queued up. >> The size of the aggregates is dynamically computed from the current >> rate: they are limited a maximum of four milliseconds of (estimated) >> airtime (for the BE queue; the others have different limits). >> >> So in a sense there's already a dynamic limit on the hardware queues. >> Now, whether four milliseconds is the right maximum aggregate size might >> be worth discussing. It is the maximum allowed by the standard. Dave and >> I have been > > Ah that may be the clue that I lacked. There's got to be a dependency > on processor speed (how quickly the system and driver can get another > packet hooked up for transmission after completions) but perhaps with > aggregates being so large in time, with full aggregates even the > slowest processors are fast enough to avoid starvation. > > If there's no aggregation, a limit of some sort is needed (probably to > prevent malfunction of the hardware/driver, but in any case to limit > excess latency). And this limit will depend on processor speed (and > will need to be autotuned at runtime). ATH_NON_AGGR_MIN_QDEPTH is 8 -- so yeah, the limit is higher if there is no aggregation. These are hard-coded values, so presumably they are large enough to keep the hardware busy on most platforms (or someone would have noticed and changed them?). So I doubt there is much to be gained to add a mechanism to dynamically tune them (between 0 and 2?). The exception being in case pulling from the mac80211 queue is too slow to keep the hardware busy at the current settings. I see no problems with this on my hardware, but that's an x86 box. I would probably hold off on the dynamic tuning until having proven that there's actually a bottleneck, though... ;) -Toke
next prev parent reply other threads:[~2016-06-19 13:50 UTC|newest] Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-17 9:09 [PATCH 0/2] ath9k: Add airtime fairness scheduler Toke Høiland-Jørgensen 2016-06-17 9:09 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-17 9:09 ` [PATCH 1/2] ath9k: use mac80211 intermediate software queues Toke Høiland-Jørgensen 2016-06-17 9:09 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-17 13:28 ` Felix Fietkau 2016-06-17 13:28 ` [ath9k-devel] " Felix Fietkau 2016-06-17 13:41 ` Tim Shepard 2016-06-17 14:08 ` [ath9k-devel] " Tim Shepard 2016-06-17 14:35 ` Felix Fietkau 2016-06-17 14:35 ` [ath9k-devel] " Felix Fietkau 2016-06-17 17:45 ` Tim Shepard 2016-06-17 17:45 ` [ath9k-devel] " Tim Shepard 2016-06-17 19:15 ` Toke Høiland-Jørgensen 2016-06-17 19:16 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-17 13:43 ` Toke Høiland-Jørgensen 2016-06-17 13:43 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-17 13:48 ` Felix Fietkau 2016-06-17 13:48 ` [ath9k-devel] " Felix Fietkau 2016-06-17 16:33 ` Felix Fietkau 2016-06-17 16:33 ` [ath9k-devel] " Felix Fietkau 2016-06-17 14:10 ` Dave Taht 2016-06-17 14:10 ` Dave Taht 2016-06-18 19:05 ` [PATCH] ath9k: Switch to using " Toke Høiland-Jørgensen 2016-06-18 19:06 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-19 3:17 ` Tim Shepard 2016-06-19 3:17 ` [ath9k-devel] " Tim Shepard 2016-06-19 8:52 ` Toke Høiland-Jørgensen 2016-06-19 8:52 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-06-19 13:39 ` Tim Shepard 2016-06-19 13:40 ` [ath9k-devel] " Tim Shepard 2016-06-19 13:50 ` Toke Høiland-Jørgensen [this message] 2016-06-19 13:50 ` Toke Høiland-Jørgensen 2016-07-03 3:52 ` Tim Shepard 2016-07-03 3:53 ` [ath9k-devel] " Tim Shepard 2016-07-04 17:46 ` Toke Høiland-Jørgensen 2016-07-04 17:47 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-06 13:23 ` Felix Fietkau 2016-07-06 13:23 ` [ath9k-devel] " Felix Fietkau 2016-07-06 14:45 ` Toke Høiland-Jørgensen 2016-07-06 14:46 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-06 16:16 ` [PATCH v2] " Toke Høiland-Jørgensen 2016-07-06 16:17 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-06 17:57 ` Sebastian Gottschall 2016-07-06 18:19 ` [ath9k-devel] " Sebastian Gottschall 2016-07-06 18:13 ` Felix Fietkau 2016-07-06 18:13 ` [ath9k-devel] " Felix Fietkau 2016-07-06 18:52 ` Toke Høiland-Jørgensen 2016-07-06 18:52 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-06 18:59 ` Felix Fietkau 2016-07-06 18:59 ` [ath9k-devel] " Felix Fietkau 2016-07-06 19:08 ` Toke Høiland-Jørgensen 2016-07-06 19:08 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-06 19:34 ` [PATCH v3] " Toke Høiland-Jørgensen 2016-07-06 19:38 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-08 14:26 ` [v3] " Kalle Valo 2016-07-08 14:26 ` [ath9k-devel] " Kalle Valo 2016-07-08 15:53 ` Toke Høiland-Jørgensen 2016-07-08 15:53 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-08 16:10 ` Felix Fietkau 2016-07-08 16:10 ` [ath9k-devel] " Felix Fietkau 2016-07-08 16:28 ` Toke Høiland-Jørgensen 2016-07-08 16:28 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-08 16:31 ` Felix Fietkau 2016-07-08 16:31 ` [ath9k-devel] " Felix Fietkau 2016-07-08 16:38 ` Toke Høiland-Jørgensen 2016-07-08 16:38 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-08 18:24 ` Sebastian Gottschall 2016-07-08 18:24 ` [ath9k-devel] " Sebastian Gottschall 2016-07-09 12:00 ` Toke Høiland-Jørgensen 2016-07-09 12:00 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-07-08 16:38 ` [PATCH v3] " Tim Shepard 2016-07-08 16:38 ` [ath9k-devel] " Tim Shepard 2016-07-09 15:44 ` Toke Høiland-Jørgensen 2016-07-09 15:45 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-08-05 16:03 ` [PATCH v4] " Toke Høiland-Jørgensen 2016-08-05 16:05 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-08-22 15:43 ` Kalle Valo 2016-08-22 15:43 ` [ath9k-devel] " Kalle Valo 2016-08-22 16:16 ` Toke Høiland-Jørgensen 2016-08-22 16:16 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-08-22 17:02 ` Kalle Valo 2016-08-22 17:02 ` [ath9k-devel] " Kalle Valo 2016-08-22 17:13 ` Toke Høiland-Jørgensen 2016-08-22 17:13 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-08-23 6:59 ` Kalle Valo 2016-08-23 6:59 ` [ath9k-devel] " Kalle Valo 2016-08-23 8:52 ` Arend van Spriel 2016-08-23 8:58 ` [ath9k-devel] " Arend van Spriel 2016-10-05 14:02 ` Toke Høiland-Jørgensen 2016-10-05 14:09 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-10-05 15:50 ` Kalle Valo 2016-10-05 15:50 ` [ath9k-devel] " Kalle Valo 2016-10-05 16:55 ` Toke Høiland-Jørgensen 2016-10-05 16:55 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-10-05 17:54 ` Kalle Valo 2016-10-05 17:54 ` [ath9k-devel] " Kalle Valo 2016-10-05 19:56 ` Toke Høiland-Jørgensen 2016-10-05 19:56 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-09-02 14:00 ` [PATCH v5] " Toke Høiland-Jørgensen 2016-09-03 10:16 ` Felix Fietkau 2016-10-07 11:43 ` [v5] " Kalle Valo 2016-11-09 2:22 ` Kalle Valo 2016-11-09 2:44 ` Tim Shepard 2016-11-09 10:42 ` Toke Høiland-Jørgensen 2016-11-09 20:07 ` Valo, Kalle 2016-11-09 11:31 ` [PATCH v6] " Toke Høiland-Jørgensen 2016-11-09 22:42 ` [v6] " Kalle Valo 2016-11-09 23:10 ` Toke Høiland-Jørgensen 2016-11-15 15:00 ` Kalle Valo 2016-06-17 9:09 ` [PATCH 2/2] ath9k: Add a per-station airtime deficit scheduler Toke Høiland-Jørgensen 2016-06-17 9:09 ` [ath9k-devel] " Toke Høiland-Jørgensen 2016-11-24 13:54 ` [PATCH v2] ath9k: Introduce airtime fairness scheduling between stations Toke Høiland-Jørgensen 2016-11-25 15:16 ` Valo, Kalle 2016-11-27 15:58 ` Toke Høiland-Jørgensen 2016-11-28 9:34 ` Felix Fietkau 2016-11-28 10:00 ` Toke Høiland-Jørgensen 2016-11-28 10:12 ` [PATCH v3] " Toke Høiland-Jørgensen 2016-12-15 8:43 ` [v3] " Kalle Valo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87twgpqodc.fsf@toke.dk \ --to=toke@toke.dk \ --cc=ath9k-devel@lists.ath9k.org \ --cc=linux-wireless@vger.kernel.org \ --cc=make-wifi-fast@lists.bufferbloat.net \ --cc=nbd@nbd.name \ --cc=shep@alum.mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.