DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0832F206B5
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Wed, 26 Sep 2018 00:09:17 -0700
From:   Rajkumar Manoharan <rmanohar@codeaurora.org>
To:     =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
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: <153711973134.9231.18038849900399644494.stgit@alrua-x1.karlstad.toke.dk>
References: <153711966150.9231.13481453399723518107.stgit@alrua-x1>
 <153711973134.9231.18038849900399644494.stgit@alrua-x1.karlstad.toke.dk>
Message-ID: <826b6251746ee4d280d532f4ecdc5aa3@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2018-09-16 10:42, Toke Høiland-Jørgensen 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.
> 
> +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);
> +
> 
This looks wrong to me. txqi->flags are protected by fq->lock but here
it is by active_txq_lock. no?

> @@ -3677,6 +3751,7 @@ void ieee80211_txq_schedule_end(struct
> ieee80211_hw *hw, u8 ac)
>  	struct ieee80211_local *local = hw_to_local(hw);
> 
>  	spin_unlock_bh(&local->active_txq_lock[ac]);
> +	tasklet_schedule(&local->wake_txqs_tasklet);
>  }
> 
It is an overload to schedule wake_txqs_tasklet for each txq unlock.
Instead I would prefer to call __ieee80211_kick_airtime from 
schedule_end.
Thoughts?

-Rajkumar