From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail2.tohojo.dk (mail2.tohojo.dk [77.235.48.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 2A5F53B260 for ; Wed, 6 Jul 2016 14:52:40 -0400 (EDT) X-Virus-Scanned: amavisd-new at mail2.tohojo.dk DKIM-Filter: OpenDKIM Filter v2.10.3 mail2.tohojo.dk 940F640472 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=201310; t=1467831157; bh=rr/Y9NnrKcVHnx3k+NVJCB6sINq4AkYO4qDCEv+xIeY=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=TFSsZaVugk0+MejZNQxeJx/l0+8qREbd0i4tjoh3jlmK06H6TQNs+hBEMBmOWIDie 8I0V/5NZAhAuqlbCb3zaO2S2XstypTvgSvaNLaE+BPtj5bnMIzyKKCVaM4ZFpYkaEN DFohCe9by0BMYCIaZGJkg5Y6IoqnHPRijyXFEVYA= Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 4A36D632C4; Wed, 6 Jul 2016 20:52:36 +0200 (CEST) From: =?utf-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= To: Felix Fietkau Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, ath9k-devel@lists.ath9k.org, Tim Shepard References: <20160618190505.24038-1-toke@toke.dk> <20160706161612.16597-1-toke@toke.dk> <85779557-6c5a-fcd3-60cf-4d3c79aec652@nbd.name> Date: Wed, 06 Jul 2016 20:52:36 +0200 In-Reply-To: <85779557-6c5a-fcd3-60cf-4d3c79aec652@nbd.name> (Felix Fietkau's message of "Wed, 6 Jul 2016 20:13:55 +0200") X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87r3b6mwej.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: , X-List-Received-Date: Wed, 06 Jul 2016 18:52:40 -0000 Felix Fietkau writes: > On 2016-07-06 18:16, Toke H=C3=B8iland-J=C3=B8rgensen 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. >>=20 >> 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). >>=20 >> Based on Tim's original patch set, but reworked quite thoroughly. >>=20 >> Cc: Tim Shepard >> Cc: Felix Fietkau >> Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen >> --- >> 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). >>=20 >> 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/wireles= s/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, struc= t ath_txq *txq, >> seqno =3D bf->bf_state.seqno; >>=20=20 >> /* 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? -Toke