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 C505F3B29E for ; Wed, 19 Sep 2018 05:09:08 -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=1537348147; bh=H2X7dFVOqKOAfvuu0BJ6wV1tEzMoZOu9IhMABjsr1E8=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=NU0gN5F2zZBW7RLtbojH9ZSu2x6PkieQrzwYvn4JFprqNz4lbNv1oFEdYzfKbrfF3 hJz5xH3qeu98e3e+TgzfXWMK9jw8n8f0Z/KnSeAdgxk0YuBUs9WjwcL/E9717Ak80C ONEy8LcLNaqzQrxQh5hPgv4s32tsvpEP7lMhGy5THL2M9JUsX+/1LHVk25KUiW4e+j /vzgeejJJpAUvCemgnY2usf81bPRejflE8VNx5YUMhtJxD17nVZ6NHujPIbhUvp3Xh YPieGtDEXpSUjzmro+TORpJCVDRJz0EtCADSoHwSjLJsgadC1NwEa5W/QKFD3BQp2z vcP5hnkFIphQQ== To: Rajkumar Manoharan Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, Felix Fietkau , Kan Yan , linux-wireless-owner@vger.kernel.org In-Reply-To: <5e2612cbd4ea9f560a63149c599f8587@codeaurora.org> References: <153711966150.9231.13481453399723518107.stgit@alrua-x1> <153711973109.9231.7094211814263758096.stgit@alrua-x1.karlstad.toke.dk> <13400b5f9bdb5e36c6afabd071cc7b0d@codeaurora.org> <87o9cvksiv.fsf@toke.dk> <87efdq8rn0.fsf@toke.dk> <5e2612cbd4ea9f560a63149c599f8587@codeaurora.org> Date: Wed, 19 Sep 2018 11:09:06 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87y3bxkg5p.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 v4 1/4] mac80211: Add TXQ scheduling API 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, 19 Sep 2018 09:09:09 -0000 Rajkumar Manoharan writes: > On 2018-09-18 13:41, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> Rajkumar Manoharan writes: >>=20 >>>>> Also an option to add the node at head or tail would be preferred.=20 >>>>> If >>>>> return_txq adds node at head of list, then it is forcing the driver=20 >>>>> to >>>>> serve same txq until it becomes empty. Also this will not allow the >>>>> driver to send N frames from each txqs. >>>>=20 >>>> The whole point of this patch set is to move those kinds of decisions >>>> out of the driver and into mac80211. The airtime scheduler won't >>>> achieve >>>> fairness if it allows queues to be queued to the end of the rotation >>>> before its deficit turns negative. And obviously there's some lag in >>>> this since we're using after-the-fact airtime information. >>>>=20 >>> Hmm.. As you know ath10k kind of doing fairness by serving fixed=20 >>> frames >>> from each txq. This approach will be removed from ath10k. >>>=20 >>>> For ath9k this has not really been a problem in my tests; if the lag >>>> turns out to be too great for ath10k (which I suppose is a=20 >>>> possibility >>>> since we don't get airtime information on every TX-compl), I figure=20 >>>> we >>>> can use the same estimated airtime value that is used for throttling >>>> the >>>> queues to adjust the deficit immediately... >>>>=20 >>> Thats true. I am porting Kan's changes of airtime estimation for each >>> msdu for firmware that does not report airtime. >>=20 >> Right. My thinking with this was that we could put the per-frame=20 >> airtime >> estimation into ieee80211_tx_dequeue(), which could track the >> outstanding airtime and just return NULL if it goes over the threshold. >> I think this is fairly straight-forward to do on its own; the biggest >> problem is probably finding the space in the mac80211 cb? >>=20 >> Is this what you are working on porting? Because then I'll wait for=20 >> your >> patch rather than starting to write this code myself :) >>=20 > Kind of.. something like below. > > tx_dequeue(){ > compute airtime_est from last_tx_rate > if (sta->airtime[ac].deficit < airtime_est) > return NULL; > dequeue skb and store airtime_est in cb > } I think I would decouple it further and not use the deficit. But rather: tx_dequeue(){ if (sta->airtime[ac].outstanding > AIRTIME_OUTSTANDING_MAX) return NULL compute airtime_est from last_tx_rate dequeue skb and store airtime_est in cb sta->airtime[ac].outstanding +=3D airtime_est; } > Unfortunately ath10k is not reporting last_tx_rate in tx_status(). So > I also applied this "ath10k: report tx rate using ieee80211_tx_status" > change. Yeah, that and the patch that computes the last used rate will probably be necessary; but they can be pretty much applied as-is, right? >> This mechanism on its own will get us the queue limiting and latency >> reduction goodness for firmwares with deep queues. And for that it can >> be completely independent of the airtime fairness scheduler, which can >> use the after-tx-compl airtime information to presumably get more >> accurate fairness which includes retransmissions etc. >>=20 >> Now, we could *also* use the ahead-of-time airtime estimation for >> fairness; either just as a fallback for drivers that can't get actual >> airtime usage information for the hardware, or as an alternative in >> cases where it works better for other reasons. But I think that >> separating the two in the initial implementation makes more sense; that >> will make it easier to experiment with different combinations of the >> two. >>=20 >> Does that make sense? :) >>=20 > Completely agree. I was thinking of using this as fallback for devices > that does not report airtime but tx rate. Great! Seems we are converging on a workable solution, then :) -Toke