From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sipsolutions.net (s3.sipsolutions.net [5.9.151.49]) (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 CD3933B260 for ; Fri, 30 Sep 2016 08:43:35 -0400 (EDT) Received: by sipsolutions.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1bpxA6-00023x-Ag; Fri, 30 Sep 2016 14:43:34 +0200 Message-ID: <1475239412.17481.59.camel@sipsolutions.net> From: Johannes Berg To: Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= Cc: make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org Date: Fri, 30 Sep 2016 14:43:32 +0200 In-Reply-To: <87y4295zpy.fsf@toke.dk> References: <20160922170420.5193-1-toke@toke.dk> <20160922170420.5193-3-toke@toke.dk> <1475231220.17481.32.camel@sipsolutions.net> <87y4295zpy.fsf@toke.dk> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: Re: [Make-wifi-fast] [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue 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: Fri, 30 Sep 2016 12:43:35 -0000 > Because I need to run it anyway for the xmit_fast path on dequeue. I > thought doing it this way simplifies the code (at the cost of the > handler getting called twice when xmit_fast is not active). Ok, that's fair. > > I *think* it should commute with the rate control handler, but even > > so, wouldn't it make more sense to have rate control late? Assuming > > the packets are queued for some amount of time, having rate control > > information queued with them would get stale. > > Yes, having rate control run at dequeue would be good, and that's > what I did initially. However, I found that this would lead to a > deadlock because the rate control handler would send out packets in > some cases (I forget the details but can go back and check if > needed). And since the dequeue function is called with the driver TXQ > lock held, that would lead to a deadlock when those packets made it > to the driver TX path. That seems really odd, but I can see how a deadlock happens then. > So I decided to just keep it this way for now; I plan to go poking > into the rate controller later anyway, so moving the handler to later > could be part of that. Sure, that's fair. > But that handler only sets a few flags? Is > tx->sdata->control_port_protocol likely to change while the packet is > queued? Oh right, I confused things there. We check the controlled port much earlier, but anyway that should be OK. > > It's a bit unfortunate that you lose fast-xmit here completely for > > the key stuff, but I don't see a good way to avoid that, other than > > completely rejiggering all the (possibly affected) queues when keys > > change... might be very complex to do that, certainly a follow-up > > patch if it's desired. > > Yeah, figured it was better to have something that's correct and then > go back and change it if the performance hit turns out to be too > high. Makes sense. > > This check seems a bit weird though - how could fast-xmit be set > > without a TXQ station? > > I think that is probably just left over from before I introduced the > control flag. Should be fine to remove it. Ok. > > > > > > > > +++ b/net/mac80211/util.c > > > @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct > > > ieee80211_txq *txq, > > >        unsigned long *byte_cnt) > > >  { > > >   struct txq_info *txqi = to_txq_info(txq); > > > + u32 frag_cnt = 0, frag_bytes = 0; > > > + struct sk_buff *skb; > > > + > > > + skb_queue_walk(&txqi->frags, skb) { > > > + frag_cnt++; > > > + frag_bytes += skb->len; > > > + } > > > > I hope this is called infrequently :) > > Well, ath10k is the only user. It does get called on each > wake_tx_queue, though, so not that infrequently. My reasoning was > that since the frags queue is never going to have more than a fairly > small number of packets in it (those produced from a single split > packet), counting this way is acceptable instead of keeping a state > variable up to date. Can change it if you disagree :) No, I guess you're right, it can't be a long queue. > Not sure if you want a v10, or if you're satisfied with the above > comments and will just fix up the nits on merging? > I'll fix it up. Thanks! johannes