Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 09 Oct 2019 16:18:20 +0800
From:   Yibo Zhao <yiboz@codeaurora.org>
To:     Kan Yan <kyan@google.com>
Cc:     johannes@sipsolutions.net, linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, toke@redhat.com, nbd@nbd.name
Subject: Re: [PATCH v2 1/2] mac80211: Implement Airtime-based Queue Limit
 (AQL)
In-Reply-To: <20191007043120.67567-2-kyan@google.com>
References: <20191007043120.67567-1-kyan@google.com>
 <20191007043120.67567-2-kyan@google.com>
Message-ID: <18630c07d0aa46d16cf660d013f96b3d@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index f13eb2f61ccf..dadb643a5498 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3669,7 +3669,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct
> ieee80211_hw *hw, u8 ac)
>  {
>  	struct ieee80211_local *local = hw_to_local(hw);
>  	struct ieee80211_txq *ret = NULL;
> -	struct txq_info *txqi = NULL;
> +	struct txq_info *txqi = NULL, *head = NULL;
> +	bool found_eligible_txq = false;
> 
>  	spin_lock_bh(&local->active_txq_lock[ac]);
> 
> @@ -3680,20 +3681,32 @@ struct ieee80211_txq
> *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
>  	if (!txqi)
>  		goto out;
> 
> +	if (txqi == head && !found_eligible_txq)
> +		goto out;

I assume here the only txq in the list that does not meet AQL check will 
not be dequeued. Right? Will it affect peak throughput once there is 
only one station.

How about dequeuing it anyway regardless AQL because it is the only one 
active now so it is fine to occupy the rest bandwidth. Otherwise, I am 
afraid next_txq() will return NULL in the test only one station is 
present.

> @@ -3748,10 +3785,10 @@ bool ieee80211_txq_may_transmit(struct 
> ieee80211_hw *hw,
>  	struct sta_info *sta;
>  	u8 ac = txq->ac;
> 
> -	spin_lock_bh(&local->active_txq_lock[ac]);
> -
>  	if (!txqi->txq.sta)
> -		goto out;
> +		return true;

why return here? I think even a txq without sta info should get removed 
from list and added it back later in return_txq() if needed. No?

> +
> +	spin_lock_bh(&local->active_txq_lock[ac]);
> 
>  	if (list_empty(&txqi->schedule_order))
>  		goto out;


-- 
Yibo