From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nbd.name (nbd.name [IPv6:2a01:4f8:131:30e2::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id E17C23B260 for ; Wed, 6 Jul 2016 15:00:00 -0400 (EDT) To: =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= References: <20160618190505.24038-1-toke@toke.dk> <20160706161612.16597-1-toke@toke.dk> <85779557-6c5a-fcd3-60cf-4d3c79aec652@nbd.name> <87r3b6mwej.fsf@toke.dk> Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, ath9k-devel@lists.ath9k.org, Tim Shepard From: Felix Fietkau Message-ID: <97e6ac26-6ec0-24ee-2834-33f0c86532f3@nbd.name> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <87r3b6mwej.fsf@toke.dk> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailman-Approved-At: Mon, 28 Nov 2016 08:47:10 -0500 Subject: Re: [Make-wifi-fast] [PATCH v2] ath9k: Switch to using mac80211 intermediate software queues. X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Date: Wed, 06 Jul 2016 19:00:01 -0000 X-Original-Date: Wed, 6 Jul 2016 20:59:55 +0200 X-List-Received-Date: Wed, 06 Jul 2016 19:00:01 -0000 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(&tid->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(&tid->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