From owner-freebsd-wireless@FreeBSD.ORG Thu Apr 12 17:30:00 2012 Return-Path: Delivered-To: freebsd-wireless@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 29A5C1065670 for ; Thu, 12 Apr 2012 17:30:00 +0000 (UTC) (envelope-from bschmidt@techwires.net) Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 7D7D38FC12 for ; Thu, 12 Apr 2012 17:29:59 +0000 (UTC) Received: by wgbds12 with SMTP id ds12so2243653wgb.31 for ; Thu, 12 Apr 2012 10:29:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:from:to:subject:date:user-agent:cc:references:in-reply-to :mime-version:content-type:content-transfer-encoding:message-id :x-gm-message-state; bh=0ycOcC1o9NqKthqHB0302BvUoebldennXa6n8CjL7D4=; b=Gj+Hz18bLNh60S+zDUvWuspnGrxC8TZFq2HvGmrfDs1VFM/A25DMnU1zPu8KvaV8+n emyTOVGXkWdqT7Q67bd9eAzlV97K4yC7PKj8YNwP7qxoPunbBauzJZFv2VPY313jgsIm C/e6g1sqShy54ssr9TcBSImxHQtIeS77zkNJTegFWw0oAA5fcs592x1Z9tkM3mLJ84Mc gEHjBmAeSDIsWzFGvaxlU5c+SzobiwHXRXRKR7IywLgdPe5WjEPBzzwW/4zp2ZPkwCd4 nGSDpQloH5O0lq7bipe7SpTvXwV6v3TEnyR9FDqy+Pc/vmdpysHOVCYhwkH5FLxqOQIb TByA== Received: by 10.180.107.101 with SMTP id hb5mr7921338wib.7.1334251798392; Thu, 12 Apr 2012 10:29:58 -0700 (PDT) Received: from amy.lab.techwires.net (dslb-088-067-211-255.pools.arcor-ip.net. [88.67.211.255]) by mx.google.com with ESMTPS id fn2sm23682123wib.0.2012.04.12.10.29.55 (version=SSLv3 cipher=OTHER); Thu, 12 Apr 2012 10:29:56 -0700 (PDT) Sender: Bernhard Schmidt From: Bernhard Schmidt To: Adrian Chadd Date: Thu, 12 Apr 2012 19:30:16 +0200 User-Agent: KMail/1.13.7 (FreeBSD/9.0-STABLE; KDE/4.7.4; amd64; ; ) References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201204121930.17011.bschmidt@freebsd.org> X-Gm-Message-State: ALoCoQnKF/E6QaYXX6tUulZQ2WLC/xNVTTDijlHgfM9jPW/EKl4gpBTFQDjBVzpXtsXs8CqtC/+E Cc: freebsd-wireless@freebsd.org Subject: Re: [RFC] net80211/ath/iwn: move ni_tx_ampdu to be per-tid, rather than per WME AC X-BeenThere: freebsd-wireless@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussions of 802.11 stack, tools device driver development." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Apr 2012 17:30:00 -0000 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