DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ADFD621480
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
To:     Rajkumar Manoharan <rmanohar@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, Felix Fietkau <nbd@nbd.name>,
        Kan Yan <kyan@google.com>, linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH RFC v4 3/4] mac80211: Add airtime accounting and scheduling to TXQs
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
Message-ID: <87pnx0haud.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Rajkumar Manoharan <rmanohar@codeaurora.org> 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