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 4DA5D3B260 for ; Wed, 6 Jul 2016 09:23:51 -0400 (EDT) To: =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= , Tim Shepard References: <87lh1hnvn6.fsf@toke.dk> Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, ath9k-devel@lists.ath9k.org From: Felix Fietkau Message-ID: <2c4c9821-fea6-fc95-f6a3-cf95e703cce6@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: <87lh1hnvn6.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] 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 13:23:51 -0000 X-Original-Date: Wed, 6 Jul 2016 15:23:49 +0200 X-List-Received-Date: Wed, 06 Jul 2016 13:23:51 -0000 On 2016-07-04 19:46, Toke Høiland-Jørgensen wrote: > Tim Shepard writes: > >> Thanks for unconfusing me a couple weeks ago, and cluing me into how >> the limit on ->pending_frames is not really relevant for the data >> packets that go through the new intermediate queues. >> >> But I'm not sure if this would allow us to remove the limit on >> pending_frames because even though normal data packets would not >> normally build up that many packets, there are other packet types >> which bypass the intermediate queues and are transmitted directly >> (also in most cases bypassing the ath9k internal queues in the way >> ath9k worked before we patched it to use the mac80211 intermediate >> queues). > > Yes, but, well, since they're not queued they are not going to overflow > anything. The aggregation building logic stops at two queued aggregates, > so the default limit of 123 packets is never going to be hit when the > queue is moved into the mac80211 layer. So keeping the knobs around only > helps people who purposefully want to cripple their ability to do > aggregation; and it won't be doing what it promises (limiting qlen), > since that is now moved out of the driver. So IMO, removing the knobs is > the right thing to do. I have already updated my patch to do so, which > I'll send as a v2 once the other bits are resolved. I agree. >> Earlier Felix said: >> >>> Channel context based queue handling can be dealt with by >>> stopping/starting relevant queues on channel context changes. >> >> But I don't see how to handle the case here where packets get passed >> to the driver with ath_tx_start() and wind up in the scenario above. > > Well, presumably the upper layers won't try to transmit anything through > the old TX path if the start/stop logic is implemented properly. The > chanctx code already seems to call the ieee80211_{start,stop}_queue() > functions when changing context, so not sure what else is needed. Guess > I'll go see if I can provoke an actual triggering of the bug, unless > Felix elaborates on what he means before I get around to it (poke, > Felix? :)). Then I guess the logic in ath_tx_start was a leftover from a time before some queue related rework happened to the chanctx code. In that case you can simply remove the chanctx related software queueing stuff from ath_tx_start. - Felix