Re: [PATCH 13/30] wifi: mac80211: don't apply CoDel to PS-buffered frames

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Johannes Berg <johannes@sipsolutions.net> writes:

> From: Johannes Berg <johannes.berg@intel.com>
>
> Frames for sleeping stations remain on the TXQs now, applying
> CoDel to them would start dropping them due to a long wait on
> dequeue, but for PS STAs they will be on the queues for a while.
> Bypass CoDel while a STA is sleeping and for a while (10s) or
> when the queue drains after it wakes up, and for frames that
> were enqueued after the wakeup as well, which really is the
> common case.
>
> Assisted-by: LLM
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/mac80211/ieee80211_i.h |  7 +++++
>  net/mac80211/sta_info.c    |  6 ++++-
>  net/mac80211/tx.c          | 52 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 5045144641a4..cb947341fee6 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -978,6 +978,7 @@ enum txq_info_flags {
>  	IEEE80211_TXQ_AMPDU,
>  	IEEE80211_TXQ_NO_AMSDU,
>  	IEEE80211_TXQ_DIRTY,
> +	IEEE80211_TXQ_PS_BACKLOG,
>  };
>  
>  /**
> @@ -990,6 +991,9 @@ enum txq_info_flags {
>   * @schedule_order: used with ieee80211_local->active_txqs
>   * @schedule_round: counter to prevent infinite loops on TXQ scheduling
>   * @flags: TXQ flags from &enum txq_info_flags
> + * @ps_backlog_end: with %IEEE80211_TXQ_PS_BACKLOG, indicates when
> + *	the station woke up, i.e. frames before this were just queued
> + *	due to powersave
>   * @txq: the driver visible part
>   */
>  struct txq_info {
> @@ -1003,6 +1007,7 @@ struct txq_info {
>  	struct sk_buff_head frags;
>  
>  	unsigned long flags;
> +	codel_time_t ps_backlog_end;
>  
>  	/* keep last! */
>  	struct ieee80211_txq txq;
> @@ -2724,6 +2729,8 @@ void ieee80211_txq_init(struct ieee80211_sub_if_data *sdata,
>  			struct txq_info *txq, int tid);
>  struct sk_buff *ieee80211_tx_dequeue_ps_release(struct ieee80211_local *local,
>  						struct txq_info *txqi);
> +void ieee80211_txq_ps_backlog_start(struct ieee80211_local *local,
> +				    struct txq_info *txqi);

The variable above is called ps_backlog_end, the function is called
ps_backlog_start. Seems a tad confusing? :)


>  void ieee80211_txq_purge(struct ieee80211_local *local,
>  			 struct txq_info *txqi);
>  void ieee80211_purge_sta_txqs(struct sta_info *sta);
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 51504f57128f..8d79e532b109 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -1889,10 +1889,14 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>  	/* this can be called in process context, e.g. for mesh */
>  	local_bh_disable();
>  	for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> +		struct txq_info *txqi;
> +
>  		if (!sta->sta.txq[i] || !txq_has_queue(sta->sta.txq[i]))
>  			continue;
>  
> -		schedule_and_wake_txq(local, to_txq_info(sta->sta.txq[i]));
> +		txqi = to_txq_info(sta->sta.txq[i]);
> +		ieee80211_txq_ps_backlog_start(local, txqi);
> +		schedule_and_wake_txq(local, txqi);
>  	}
>  	local_bh_enable();
>  
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 0bd324a4d4cb..856018cdb617 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1380,6 +1380,55 @@ static void codel_drop_func(struct sk_buff *skb,
>  	ieee80211_free_txskb(hw, skb);
>  }
>  
> +/* bypass codel since PS can last a long time */
> +static bool ieee80211_txq_codel_bypass(struct txq_info *txqi,
> +				       const struct fq_flow *flow)
> +{
> +	struct sta_info *sta;
> +	struct sk_buff *skb;
> +
> +	if (!txqi->txq.sta)
> +		return false;
> +
> +	sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +	if (test_sta_flag(sta, WLAN_STA_PS_STA) ||
> +	    test_sta_flag(sta, WLAN_STA_PS_DRIVER) ||
> +	    test_sta_flag(sta, WLAN_STA_PS_DELIVER))
> +		return true;
> +
> +	if (likely(!test_bit(IEEE80211_TXQ_PS_BACKLOG, &txqi->flags)))
> +		return false;
> +
> +	/*
> +	 * Stop treating as backlog after it cleared or after 1s
> +	 * (the latter to avoid time wrap and issues from that).
> +	 */
> +#define IEEE80211_TXQ_MAX_PS_BACKLOG	MS2TIME(MSEC_PER_SEC)
> +
> +	if (!txqi->tin.backlog_packets ||
> +	    codel_time_after(codel_get_time(),
> +			     (codel_time_t)(txqi->ps_backlog_end +
> +					    IEEE80211_TXQ_MAX_PS_BACKLOG))) {
> +		clear_bit(IEEE80211_TXQ_PS_BACKLOG, &txqi->flags);
> +		return false;
> +	}
> +
> +	skb = skb_peek(&flow->queue);
> +	return skb && codel_time_before(codel_skb_time_func(skb),
> +					txqi->ps_backlog_end);
> +}
> +
> +void ieee80211_txq_ps_backlog_start(struct ieee80211_local *local,
> +				    struct txq_info *txqi)
> +{
> +	spin_lock_bh(&local->fq.lock);
> +	if (txqi->tin.backlog_packets) {
> +		txqi->ps_backlog_end = codel_get_time();
> +		set_bit(IEEE80211_TXQ_PS_BACKLOG, &txqi->flags);

We should probably clear the codel variables here as well? Otherwise, if
CoDel was already in a dropping state when this happens, that can carry
over to after the bypass ends.

-Toke