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:   Thu, 10 Oct 2019 10:43:49 +0800
From:   Yibo Zhao <yiboz@codeaurora.org>
To:     Felix Fietkau <nbd@nbd.name>
Cc:     linux-wireless@vger.kernel.org, johannes@sipsolutions.net,
        =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
        linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH] mac80211: rework locking for txq scheduling / airtime
 fairness
In-Reply-To: <20190315100335.91445-1-nbd@nbd.name>
References: <20190315100335.91445-1-nbd@nbd.name>
Message-ID: <b2bded992cb02bad84fc738b7e4b1837@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 ca23abbf5c0b..51cc37802439 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -3653,16 +3653,17 @@ EXPORT_SYMBOL(ieee80211_tx_dequeue);
>  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;
> 
> -	lockdep_assert_held(&local->active_txq_lock[ac]);
> +	spin_lock_bh(&local->active_txq_lock[ac]);
> 
>   begin:
>  	txqi = list_first_entry_or_null(&local->active_txqs[ac],
>  					struct txq_info,
>  					schedule_order);
>  	if (!txqi)
> -		return NULL;
> +		goto out;
> 
>  	if (txqi->txq.sta) {
>  		struct sta_info *sta = container_of(txqi->txq.sta,
> @@ -3679,21 +3680,25 @@ struct ieee80211_txq
> *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
> 
> 
>  	if (txqi->schedule_round == local->schedule_round[ac])
> -		return NULL;
> +		goto out;
> 
>  	list_del_init(&txqi->schedule_order);
>  	txqi->schedule_round = local->schedule_round[ac];
> -	return &txqi->txq;
> +	ret = &txqi->txq;
> +
> +out:
> +	spin_unlock_bh(&local->active_txq_lock[ac]);
> +	return ret;
>  }
>  EXPORT_SYMBOL(ieee80211_next_txq);
> 
> -void ieee80211_return_txq(struct ieee80211_hw *hw,
> -			  struct ieee80211_txq *txq)
> +void ieee80211_schedule_txq(struct ieee80211_hw *hw,
> +			    struct ieee80211_txq *txq)
>  {
>  	struct ieee80211_local *local = hw_to_local(hw);
>  	struct txq_info *txqi = to_txq_info(txq);
> 
> -	lockdep_assert_held(&local->active_txq_lock[txq->ac]);
> +	spin_lock_bh(&local->active_txq_lock[txq->ac]);
> 
>  	if (list_empty(&txqi->schedule_order) &&
>  	    (!skb_queue_empty(&txqi->frags) || txqi->tin.backlog_packets)) {
> @@ -3713,17 +3718,7 @@ void ieee80211_return_txq(struct ieee80211_hw 
> *hw,
>  			list_add_tail(&txqi->schedule_order,
>  				      &local->active_txqs[txq->ac]);
>  	}
> -}
> -EXPORT_SYMBOL(ieee80211_return_txq);
> 
> -void ieee80211_schedule_txq(struct ieee80211_hw *hw,
> -			    struct ieee80211_txq *txq)
> -	__acquires(txq_lock) __releases(txq_lock)
> -{
> -	struct ieee80211_local *local = hw_to_local(hw);
> -
> -	spin_lock_bh(&local->active_txq_lock[txq->ac]);
> -	ieee80211_return_txq(hw, txq);
>  	spin_unlock_bh(&local->active_txq_lock[txq->ac]);
>  }
>  EXPORT_SYMBOL(ieee80211_schedule_txq);
> @@ -3736,7 +3731,7 @@ bool ieee80211_txq_may_transmit(struct 
> ieee80211_hw *hw,
>  	struct sta_info *sta;
>  	u8 ac = txq->ac;
> 
> -	lockdep_assert_held(&local->active_txq_lock[ac]);
> +	spin_lock_bh(&local->active_txq_lock[ac]);
> 
>  	if (!txqi->txq.sta)
>  		goto out;
> @@ -3766,34 +3761,27 @@ bool ieee80211_txq_may_transmit(struct 
> ieee80211_hw *hw,
> 
>  	sta->airtime[ac].deficit += sta->airtime_weight;
>  	list_move_tail(&txqi->schedule_order, &local->active_txqs[ac]);
> +	spin_unlock_bh(&local->active_txq_lock[ac]);
> 
>  	return false;
>  out:
>  	if (!list_empty(&txqi->schedule_order))
>  		list_del_init(&txqi->schedule_order);
> +	spin_unlock_bh(&local->active_txq_lock[ac]);
> 
>  	return true;
>  }
>  EXPORT_SYMBOL(ieee80211_txq_may_transmit);
> 
>  void ieee80211_txq_schedule_start(struct ieee80211_hw *hw, u8 ac)
> -	__acquires(txq_lock)
>  {
>  	struct ieee80211_local *local = hw_to_local(hw);
> 
>  	spin_lock_bh(&local->active_txq_lock[ac]);
>  	local->schedule_round[ac]++;
Hi Felix,

In ath10k, we might have situations like,

           CPU0                           CPU1
...                                   ieee80211_txq_schedule_start()
...                                   while(ieee80211_next_txq()) {
...                                       driver dequeue packets from 
txq
...                                       ieee80211_return_txq()
ieee80211_txq_schedule_start()            ...
ieee80211_next_txq()                  }
driver dequeue packets from txq       ieee80211_txq_schedule_end()
ieee80211_return_txq()
ieee80211_txq_schedule_end()

The problem is while CPU1 is looping to dequeue txqs, the schedule_round 
which is used to make sure no infinite loop will be changed in CPU0. So 
a txq has already been scheduled will be scheduled again then the loop 
won't be end at that point as our expected.

> -}
> -EXPORT_SYMBOL(ieee80211_txq_schedule_start);
> -
> -void ieee80211_txq_schedule_end(struct ieee80211_hw *hw, u8 ac)
> -	__releases(txq_lock)
> -{
> -	struct ieee80211_local *local = hw_to_local(hw);
> -
>  	spin_unlock_bh(&local->active_txq_lock[ac]);
>  }
> -EXPORT_SYMBOL(ieee80211_txq_schedule_end);
> +EXPORT_SYMBOL(ieee80211_txq_schedule_start);
> 
>  void __ieee80211_subif_start_xmit(struct sk_buff *skb,
>  				  struct net_device *dev,

-- 
Yibo