[RFC] net80211/ath/iwn: move ni_tx_ampdu to be per-tid, rather than per WME AC

[Bernhard Schmidt]

On Wednesday 11 April 2012 04:53:27 Adrian Chadd wrote:
> Hi,
> 
> This patch changes ni_tx_ampdu to be per-TID rather than per-AC.
> 
> It includes:
> 
> * changes to net80211 to use the QoS frame TID, rather than
> calculating it from the AC (which may have been calculated from the
> TID in the first place);
> * the one changes to ath(4) to use it;
> * a couple of changes to iwn which was using ni_tx_ampdu state.
> 
> I'd like to commit this to -HEAD.

Basically fine, but a few nits code be fixed, see below.
 
> Index: sys/net80211/ieee80211_ht.c
> ===================================================================
> --- sys/net80211/ieee80211_ht.c	(revision 234108)
> +++ sys/net80211/ieee80211_ht.c	(working copy)
> @@ -1198,9 +1199,10 @@
>  	ni->ni_htopmode = 0;		/* XXX need protection state */
>  	ni->ni_htstbc = 0;		/* XXX need info */
>  
> -	for (ac = 0; ac < WME_NUM_AC; ac++) {
> -		tap = &ni->ni_tx_ampdu[ac];
> -		tap->txa_ac = ac;
> +	for (tid = 0; tid < WME_NUM_TID; tid++) {
> +		tap = &ni->ni_tx_ampdu[tid];
> +		tap->txa_ac = TID_TO_WME_AC(tid);
> +		tap->txa_tid = tid;

Do we really need both, txa_ac and txa_tid?

> @@ -2099,7 +2101,7 @@
>  	tap->txa_flags &= ~IEEE80211_AGGR_NAK;
>  
>  	dialogtoken = (tokens+1) % 63;		/* XXX */
> -	tid = WME_AC_TO_TID(tap->txa_ac);
> +	tid = tap->txa_tid;
>  	tap->txa_start = ni->ni_txseqs[tid];

Better use txa_tid directly, no?

> @@ -2292,7 +2294,7 @@
>  	IEEE80211_ADDR_COPY(bar->i_ra, ni->ni_macaddr);
>  	IEEE80211_ADDR_COPY(bar->i_ta, vap->iv_myaddr);
>  
> -	tid = WME_AC_TO_TID(tap->txa_ac);
> +	tid = tap->txa_tid;

same here

> Index: sys/net80211/ieee80211_ht.h
> ===================================================================
> --- sys/net80211/ieee80211_ht.h	(revision 234108)
> +++ sys/net80211/ieee80211_ht.h	(working copy)
> @@ -45,6 +45,7 @@
>  #define	IEEE80211_AGGR_NAK		0x0010	/* peer NAK'd ADDBA request */
>  #define	IEEE80211_AGGR_BARPEND		0x0020	/* BAR response pending */
>  	uint8_t		txa_ac;
> +	uint8_t		txa_tid;

Again, do we need both?

>  	uint8_t		txa_token;	/* dialog token */
>  	int		txa_lastsample;	/* ticks @ last traffic sample */
>  	int		txa_pkts;	/* packets over last sample interval */
> Index: sys/net80211/ieee80211_output.c
> ===================================================================
> --- sys/net80211/ieee80211_output.c	(revision 234108)
> +++ sys/net80211/ieee80211_output.c	(working copy)
> @@ -324,7 +324,8 @@
>  		    (vap->iv_flags_ht & IEEE80211_FHT_AMPDU_TX) &&
>  		    (m->m_flags & M_EAPOL) == 0) {
>  			const int ac = M_WME_GETAC(m);
> -			struct ieee80211_tx_ampdu *tap = &ni->ni_tx_ampdu[ac];
> +			const int tid = WME_AC_TO_TID(ac);
> +			struct ieee80211_tx_ampdu *tap = &ni->ni_tx_ampdu[tid];

Use WME_AC_TO_TID() directly?

>  			ieee80211_txampdu_count_packet(tap);
>  			if (IEEE80211_AMPDU_RUNNING(tap)) {
> Index: sys/net80211/ieee80211_superg.c
> ===================================================================
> --- sys/net80211/ieee80211_superg.c	(revision 234108)
> +++ sys/net80211/ieee80211_superg.c	(working copy)
> @@ -562,9 +562,11 @@
>  	IEEE80211_LOCK(ic);
>  	head = sq->head;
>  	while ((m = sq->head) != NULL && M_AGE_GET(m) < quanta) {
> +		int ac = M_WME_GETAC(m);
> +		int tid = WME_AC_TO_TID(ac);

No need for those 2 here, use em directly below.

>  		/* clear tap ref to frame */
>  		ni = (struct ieee80211_node *) m->m_pkthdr.rcvif;
> -		tap = &ni->ni_tx_ampdu[M_WME_GETAC(m)];
> +		tap = &ni->ni_tx_ampdu[tid];
>  		KASSERT(tap->txa_private == m, ("staging queue empty"));
>  		tap->txa_private = NULL;
>  
> @@ -661,6 +663,7 @@
>  	struct ieee80211_tx_ampdu *tap;
>  	struct mbuf *mstaged;
>  	uint32_t txtime, limit;
> +	int tid = WME_AC_TO_TID(pri);
>  
>  	/*
>  	 * Check if the supplied frame can be aggregated.
> @@ -670,7 +673,7 @@
>  	 *     be aggregated with other types of frames when encryption is on?
>  	 */
>  	IEEE80211_LOCK(ic);
> -	tap = &ni->ni_tx_ampdu[pri];
> +	tap = &ni->ni_tx_ampdu[tid];

Use WME_AC_TO_TID() directly here.
Any reason why ac is called pri here?

>  	mstaged = tap->txa_private;		/* NB: we reuse AMPDU state */
>  	ieee80211_txampdu_count_packet(tap);
>  
> @@ -783,12 +786,13 @@
>  	struct ieee80211_superg *sg = ic->ic_superg;
>  	struct ieee80211_tx_ampdu *tap;
>  	struct mbuf *m, *head;
> -	int ac;
> +	int tid;
>  
>  	IEEE80211_LOCK(ic);
>  	head = NULL;
> -	for (ac = 0; ac < WME_NUM_AC; ac++) {
> -		tap = &ni->ni_tx_ampdu[ac];
> +	for (tid = 0; tid < WME_NUM_TID; tid++) {
> +		int ac = TID_TO_WME_AC(tid);
> +		tap = &ni->ni_tx_ampdu[tid];

I'd prefer to use TID_TO_WME_AC() directly

> Index: sys/net80211/ieee80211_ddb.c
> ===================================================================
> --- sys/net80211/ieee80211_ddb.c	(revision 234108)
> +++ sys/net80211/ieee80211_ddb.c	(working copy)
> @@ -293,7 +293,7 @@
>  		ni->ni_htopmode, ni->ni_htstbc, ni->ni_chw);
>  
>  	/* XXX ampdu state */
> -	for (i = 0; i < WME_NUM_AC; i++)
> +	for (i = 0; i < WME_NUM_TID; i++)
>  		if (ni->ni_tx_ampdu[i].txa_flags & IEEE80211_AGGR_SETUP)
>  			_db_show_txampdu("\t", i, &ni->ni_tx_ampdu[i]);
>  	for (i = 0; i < WME_NUM_TID; i++)

Might want to merge those 2 loops

> Index: sys/dev/iwn/if_iwn.c
> ===================================================================
> --- sys/dev/iwn/if_iwn.c	(revision 234108)
> +++ sys/dev/iwn/if_iwn.c	(working copy)
> @@ -3309,9 +3309,12 @@
>  	}
>  	ac = M_WME_GETAC(m);
>  
> +	/*
> +	 * XXX what about the non-QoS TID?
> +	 */

I don't know what that comment is supposed to mean, but it is
definitely wrong here. What the code below does is basically
updating the seqno, which is already done for !QoS frames in 
net80211.

>  	if (IEEE80211_QOS_HAS_SEQ(wh) &&
> -	    IEEE80211_AMPDU_RUNNING(&ni->ni_tx_ampdu[ac])) {
> -		struct ieee80211_tx_ampdu *tap = &ni->ni_tx_ampdu[ac];
> +	    IEEE80211_AMPDU_RUNNING(&ni->ni_tx_ampdu[tid])) {
> +		struct ieee80211_tx_ampdu *tap = &ni->ni_tx_ampdu[tid];
>  
>  		ring = &sc->txq[*(int *)tap->txa_private];
>  		*(uint16_t *)wh->i_seq =

-- 
Bernhard