From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.toke.dk (mail.toke.dk [IPv6:2001:470:dc45:1000::1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 3333A3CB37 for ; Wed, 26 Sep 2018 05:22:37 -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=1537953755; bh=kVEyBNxjXCiEhRnJtO363TPunYLUfKSE1UDeWPc1L7E=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=XS69LDGZqgBAq9OzOA/5Vjj1gZi0bhzsOkVfdEDpVnDxBEzB64ooa+7xslDV6Hbdf /kXeWSZB5LzG18RPeVypkXeAZE7ObnN2TVW9yhYChYDVBVj3ZRRmplc+H6V/wZ4UGs fgyPqwHQVNJTOysIOE/x5WOk7U5h4ZRXOCqjQNKqRG7bztYXOJuFnPFlMyX8tCvoBr V/ZqTBjR3c+moE8yBpaUHCG9zwI0SlUKd5m5T24gaXXiHXH/sHNFh82kCrYVQ3ze4b hURgdX4X/bNwWFc6l3RETzmrop0K7+TwTpVSnKrHDjFajELYL21EQV1s7E+XWBjnL8 e9JOqfGQttIEA== 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: <826b6251746ee4d280d532f4ecdc5aa3@codeaurora.org> References: <153711966150.9231.13481453399723518107.stgit@alrua-x1> <153711973134.9231.18038849900399644494.stgit@alrua-x1.karlstad.toke.dk> <826b6251746ee4d280d532f4ecdc5aa3@codeaurora.org> Date: Wed, 26 Sep 2018 11:22:34 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87pnx0haud.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 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, 26 Sep 2018 09:22:37 -0000 Rajkumar Manoharan writes: > On 2018-09-16 10:42, 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 >> +bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw, >> + struct ieee80211_txq *txq) >> +{ > > [...] > >> + if (ret) { >> + clear_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags); >> + list_del_init(&txqi->schedule_order); >> + } else >> + set_bit(IEEE80211_TXQ_AIRTIME_THROTTLE, &txqi->flags); >> + >>=20 > This looks wrong to me. txqi->flags are protected by fq->lock but here > it is by active_txq_lock. no? Both set_bit() and clear_bit() are atomic operations, so they don't need separate locking. See Documentation/atomic_bitops.txt >> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct >> ieee80211_hw *hw, u8 ac) >> struct ieee80211_local *local =3D hw_to_local(hw); >>=20 >> spin_unlock_bh(&local->active_txq_lock[ac]); >> + tasklet_schedule(&local->wake_txqs_tasklet); >> } >>=20 > It is an overload to schedule wake_txqs_tasklet for each txq unlock. > Instead I would prefer to call __ieee80211_kick_airtime from=20 > schedule_end. > Thoughts? Yeah, I realise scheduling the whole wake_txqs_tasklet is maybe a bit heavy-handed here. However just calling into __ieee80211_kick_airtime() means we'll end up recursing back to the same place, which is not good either (we could in theory flip-flop between two queues until we run out of stack space). My "backup plan" if the wake_txqs_tasklet turns out to be too heavy was to define a new tasklet just for this use; but wanted to see if this actually turned out to be a problem first... -Toke