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 7B2423B260 for ; Wed, 6 Jul 2016 15:08:34 -0400 (EDT) X-Virus-Scanned: amavisd-new at mail2.tohojo.dk DKIM-Filter: OpenDKIM Filter v2.10.3 mail2.tohojo.dk 1EBCA40472 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=201310; t=1467832112; bh=iPQYMrwyRPv/kKZbJr4nentsZlV20H8R+j/SGkRCpPU=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=p6CSgZboXYsGjPInJhxhytj4pza+1x2bMzbXZ28TgEBw8id0c5zyyej9JhnHMivVq G1X2DDsGf7P+CB0PpkZ+gtjBOHQ5qb2kH7rwBdAGSwrizrAQKCby3RbazDz9fA5iTc c3TJR5s6pLRT3dOsIlxgtDHY3RkOnbSJkVd4A4ww= Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id D3AC6632D2; Wed, 6 Jul 2016 21:08:30 +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> <87r3b6mwej.fsf@toke.dk> <97e6ac26-6ec0-24ee-2834-33f0c86532f3@nbd.name> Date: Wed, 06 Jul 2016 21:08:30 +0200 In-Reply-To: <97e6ac26-6ec0-24ee-2834-33f0c86532f3@nbd.name> (Felix Fietkau's message of "Wed, 6 Jul 2016 20:59:55 +0200") X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87mvlumvo1.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 19:08:34 -0000 Felix Fietkau writes: > On 2016-07-06 20:52, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Felix Fietkau writes: >>=20 >>> 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 wh= en >>>> 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! >>=20 >> Thanks :) >>=20 >>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wirel= ess/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, str= uct 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. >>=20 >> 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