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 72F153B29E for ; Wed, 10 Oct 2018 07:15:58 -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=1539170156; bh=l83TSPFgOA7/iNgoTuJGHFDhFN3+yZZdTthqNkKsqME=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=DG6bRZlBZ3CnF73Zj2tBcrIgPZj8OsSspLc3cngJhsMvgaAA1ATheOcXoCo8l3kTl lfpYV4+1zcpeDICh5Lb4u5RlHRQhX0cq3uYj9AYfKa/h6XpdtsX1EFgNwA4D+wp5T7 QRbcdjSrzK6ddhiX8o8c4o/n7XRJzscRN7NWnr+S48R1jNvPcVrvV0v+IRa5HvBlmy hplxI/mJvUHcWlbHIiYMYNPwjyWtsZd/weFlqOh7Nupz6d4UDacHMbJQTckphcg9JD GVeNu7T2+Ov1KZPGjpO9JuOJj/aOpj3LqTlUqop7AtnAxwahI733GLf4jABQJyuUrp +YeW5KNhFIJEw== To: Rajkumar Manoharan Cc: linux-wireless@vger.kernel.org, make-wifi-fast@lists.bufferbloat.net, Felix Fietkau , Kan Yan In-Reply-To: References: <153908805217.9471.9290979918041653328.stgit@alrua-kau> <153908837900.9471.5394468800857658136.stgit@alrua-kau> Date: Wed, 10 Oct 2018 13:15:55 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87zhvm832s.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 v5 3/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, 10 Oct 2018 11:15:58 -0000 Rajkumar Manoharan writes: > On 2018-10-09 05:32, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> This adds airtime accounting and scheduling to the mac80211 TXQ >> scheduler. A new callback, ieee80211_sta_register_airtime(), is added >> that drivers can call to report airtime usage for stations. >>=20 >> When airtime information is present, mac80211 will schedule TXQs >> (through ieee80211_next_txq()) in a way that enforces airtime fairness >> between active stations. This scheduling works the same way as the=20 >> ath9k >> in-driver airtime fairness scheduling. If no airtime usage is reported >> by the driver, the scheduler will default to round-robin scheduling. >>=20 >> For drivers that don't control TXQ scheduling in software, a new API >> function, ieee80211_txq_may_transmit(), is added which the driver can=20 >> use >> to check if the TXQ is eligible for transmission, or should be=20 >> throttled to >> enforce fairness. Calls to this function must also be enclosed in >> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.=20 >> TXQs >> that are throttled by ieee802111_txq_may_transmit() will be woken up=20 >> again >> by a check added to the ieee80211_wake_txqs() tasklet. >>=20 > > Toke, > > I am observing soft lockup issues again with this new series while > running traffic with 50 clients. I am continuing testing with earlier > series along with snippet I shared. Are these new lockups (that was not in your patched previous version), or did I just not get all your lock-related fixes incorporated? > When driver operates in pull-mode, throttled txqs are marked and > refilled in airtime_tasklet. This is causing major throughput drops > and packet loss and I am suspecting the latency in replenishing > deficit. Whereas in push-mode or in ath9k model, refill happens > quicker at every packet indication as well as tx completion. Yeah, the tasklet shouldn't be the main source of deficit replenishing. Can see why that would give bad performance :) > I am planning to get rid of tasklet completely as it is only meant for > pull-mode. It would be better to refill in may_transmit() itself. Hmm, right. So the way to do this correctly (from a fairness point of view) would be something like this (in max_tx()): if (this_txq.stn.deficit > 0) return true; else if (any queued TXQ currently have positive deficit) return false; /* other TXQ should try may_tx() later and get permission */ else /* all deficits < 0 */ return replenish_deficits(this_txq); And replenish_deficits() would be something like: replenish_deficits(this_txq) { repeat: for (txq in queued txqs) { txq.stn.deficit +=3D stn.weight; if (txq.stn.deficit > 0 && !wake_txq) wake_txq =3D txq; } if not wake_txq: goto repeat; if (this_txq.stn.deficit > 0) return true; else drv_wake_tx_queue(wake_txq); } The wake_tx_queue call may have to be delegated to a tasklet still, to avoid the infinite recursion problem I mentioned earlier. But the tasklet could be made simpler and wouldn't have to be called so often... Does the above make sense? -Toke