From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.toke.dk (mail.toke.dk [52.28.52.200]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 9A60B3B2A4 for ; Wed, 12 Sep 2018 07:10:21 -0400 (EDT) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=20161023; t=1536750620; bh=B6jqc3Lp2vr+iZ0s37vMF0NjaVtTwdTmc+cTAW69Oc0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Mho8W5S/EWz32hQVkj38zDYbfHvzgMb3G+O/xWm7lnLdVnj6WtoJ5Qn2D/w0ccwkp hAY3eDkFOIHvx07a23FukOE9LNECL+2G3NgEQZfe1N2QIaRwgFDt2P8sXzP+9+VScK LHGz5G9VmCsR0W2vcAyW9W5JIMbomd+oOke85KNeXJp4PiFI8mnPMK2MvqHooH2tgy wJF+OepzpZg3DVnew1dbvJcfIQv/GzRdVgmeGUWsDuq+C+VBKR0yJhOQCyForhZnMS AuxQlOscKOVMISDGrFF+WO/Q3hs67lPvX2QjY8+4W/yTDvTaGJhV92PcTVTACt2Eyc zSs51Pjrdj1xw== To: Rajkumar Manoharan Cc: Johannes Berg , linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, Felix Fietkau In-Reply-To: References: <153635803319.14170.10011969968767927187.stgit@alrua-x1> <153635897061.14170.17999956909203163571.stgit@alrua-x1> <1536567496.3224.36.camel@sipsolutions.net> <87h8ixli4s.fsf@toke.dk> Date: Wed, 12 Sep 2018 13:10:19 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87o9d3hsys.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 RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs 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, 12 Sep 2018 11:10:21 -0000 Rajkumar Manoharan writes: > On 2018-09-10 04:13, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Johannes Berg writes: >>>> - txqi->flags & (1<>>> - txqi->flags & (1<>>> - txqi->flags & (1<>>> ""); >>>> + txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP" : "RUN", >>>> + txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? " AMPDU" : "", >>>> + txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? " NO-AMSDU"=20 >>>> : ""); >>>=20 >>> consider BIT() instead as a cleanup? :) >>>=20 >>> (or maybe this is just a leftover from merging some other patches?) >>=20 >> Yeah, this is a merging artifact; Rajkumar's patch added another flag, >> that I removed again. Didn't notice that there was still a whitespace >> change in this patch... >>=20 > I added the flag based on our last discussion. The driver needs to check > txq status for each tx_dequeue(). One time txq check is not sufficient > as it allows the driver to dequeue all frames from txq. > > drv_func() { > while (ieee80211_airtime_may_transmit(txq) && > hw_has_space() && > (pkt =3D ieee80211_tx_dequeue(hw, txq))) > push_to_hw(pkt); > } Yeah, but with airtime only being recorded on TX completion, the odds of the value changing within that loop are quite low; so it's not going to work, which is why I removed it. However, after reading Kan's patches I get where you're coming from; a check in tx_dequeue() is needed for the BQL-style queue limiting. Will try to incorporate a version of that in the next series so you can see what I mean when I say it should be orthogonal; and I'm still not sure it needs a flag :) >>>> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw, >>>> + struct ieee80211_txq *txq) >>>> +{ >>>> + struct ieee80211_local *local =3D hw_to_local(hw); >>>> + struct txq_info *txqi =3D to_txq_info(txq); >>>> + bool may_tx =3D false; >>>> + >>>> + spin_lock_bh(&local->active_txq_lock); >>>> + >>>> + if (ieee80211_txq_check_deficit(local, txqi)) { >>>> + may_tx =3D true; >>>> + list_del_init(&txqi->schedule_order); >>>=20 > > To handle above case, may_transmit should remove the node only > when it is in list. > > if (list_empty(&txqi->schedule_order)) > list_del_init(&txqi->schedule_order); I assume you missed a ! in that if, right? :) > So that it can be used to determine whether txq is running negative. But still not sure what you mean here? -Toke