[prev in list] [next in list] [prev in thread] [next in thread]
List: openbsd-bugs
Subject: Re: ral(4) leaks mbufs instead of setting oactive
From: Richard Procter <richard.n.procter () gmail ! com>
Date: 2016-02-29 9:20:33
Message-ID: alpine.BSO.2.20.1602292125390.12372 () sage ! localdomain
[Download RAW message or body]
On Thu, 11 Feb 2016, Richard Procter wrote:
> [...] I found a benign off-by-one error in [my] code that left a
> tx descriptor unused. [...]
>
> I've not tested this tweak as I'm unable to access the hardware for a few
> weeks, unfortunately.
Now tested, via concurrent
$ cat /dev/zero | nc sink sink-port
# tcpdump -i ral0 -n not port 22
on the ral(4) machine over a ssh session. This fills the tx ring on my
alix 2d2.
Updated patch below fixes a comment and removes the second KASSERT.
best,
Richard.
ral0 at pci0 dev 12 function 0 "Ralink RT2860" rev 0x00: irq 9, address xx:xx:xx:xx:xx:xx
ral0: MAC/BBP RT2860 (rev 0x0103), RF RT2850 (MIMO 2T3R)
------------------------------------------------
* fix watchdog timeouts and dropped frames under load.
- on full tx ring, ring->cur wraps to an active tx descriptor. Passing
that wrapped value to the card was observed to cause general flakiness.
Fix prevents the wrap at the cost of reducing usable tx descriptors by
one.
Index: sys/dev/ic/rt2860.c
===================================================================
--- sys.orig/dev/ic/rt2860.c
+++ sys/dev/ic/rt2860.c
@@ -1171,7 +1171,7 @@ rt2860_tx_intr(struct rt2860_softc *sc,
}
sc->sc_tx_timer = 0;
- if (ring->queued < RT2860_TX_RING_COUNT)
+ if (ring->queued < RT2860_TX_RING_MAX)
sc->qfullmsk &= ~(1 << qid);
ifq_clr_oactive(&ifp->if_snd);
rt2860_start(ifp);
@@ -1618,7 +1618,7 @@ rt2860_tx(struct rt2860_softc *sc, struc
/* determine how many TXDs are required */
ntxds = 1 + (data->map->dm_nsegs / 2);
- if (ring->queued + ntxds >= RT2860_TX_RING_COUNT) {
+ if (ring->queued + ntxds >= RT2860_TX_RING_MAX) {
/* not enough free TXDs, force mbuf defrag */
bus_dmamap_unload(sc->sc_dmat, data->map);
error = EFBIG;
@@ -1656,7 +1656,7 @@ rt2860_tx(struct rt2860_softc *sc, struc
/* determine how many TXDs are now required */
ntxds = 1 + (data->map->dm_nsegs / 2);
- if (ring->queued + ntxds >= RT2860_TX_RING_COUNT) {
+ if (ring->queued + ntxds >= RT2860_TX_RING_MAX) {
/* this is a hopeless case, drop the mbuf! */
bus_dmamap_unload(sc->sc_dmat, data->map);
m_freem(m);
@@ -1714,7 +1714,7 @@ rt2860_tx(struct rt2860_softc *sc, struc
ring->cur = (ring->cur + 1) % RT2860_TX_RING_COUNT;
ring->queued += ntxds;
- if (ring->queued >= RT2860_TX_RING_COUNT)
+ if (ring->queued >= RT2860_TX_RING_MAX)
sc->qfullmsk |= 1 << qid;
/* kick Tx */
Index: sys/dev/ic/rt2860var.h
===================================================================
--- sys.orig/dev/ic/rt2860var.h
+++ sys/dev/ic/rt2860var.h
@@ -17,8 +17,9 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
-#define RT2860_TX_RING_COUNT 64
#define RT2860_RX_RING_COUNT 128
+#define RT2860_TX_RING_COUNT 64
+#define RT2860_TX_RING_MAX (RT2860_TX_RING_COUNT - 1)
#define RT2860_TX_POOL_COUNT (RT2860_TX_RING_COUNT * 2)
#define RT2860_MAX_SCATTER ((RT2860_TX_RING_COUNT * 2) - 1)
------------------------------------------------
* replace custom defrag with m_defrag()
- This fixes an error in the existing code: the "hopeless case" guard
equivales 'ring now full', so oactive is never set: the code drops any mbuf
that would fill the ring. This occurs often in practice.
- The preceding patch allows the ring to fill safely.
- The new code avoids some hoop-jumping. Currently, one tx dma-map can fill
the tx ring. Therefore an mbuf that fits a dma-map may yet not fit into the tx
ring's remaining space. To be sure it can, we must in general count the
mbuf's fragments and, if necessary, defrag it and reload the dmamap.
The new code limits the dmamap to cover at most 8 tx descriptors (= 15
fragments): now, if an mbuf fits a dma-map it will fit any ring with at least
8 free descriptors. So we need only check for 8 free descriptors and are
longer obliged to count fragments and jump hoops. The cost is unused tx
ring descriptors, at most 7 of 63, when a one-fragment mbuf occupies one
descriptor in the last block of 8.
- For simplicity on error return, shift responsibilty for calling
m_freem() to rt2860_tx()'s caller (which already calls
ieee80211_release_node()).
Index: sys/dev/ic/rt2860.c
===================================================================
--- sys.orig/dev/ic/rt2860.c
+++ sys/dev/ic/rt2860.c
@@ -567,7 +567,7 @@ rt2860_alloc_tx_pool(struct rt2860_softc
error = bus_dmamap_create(sc->sc_dmat, MCLBYTES,
RT2860_MAX_SCATTER, MCLBYTES, 0, BUS_DMA_NOWAIT,
- &data->map);
+ &data->map); /* <0> */
if (error != 0) {
printf("%s: could not create DMA map\n",
sc->sc_dev.dv_xname);
@@ -1171,7 +1171,7 @@ rt2860_tx_intr(struct rt2860_softc *sc,
}
sc->sc_tx_timer = 0;
- if (ring->queued < RT2860_TX_RING_MAX)
+ if (ring->queued <= RT2860_TX_RING_ONEMORE)
sc->qfullmsk &= ~(1 << qid);
ifq_clr_oactive(&ifp->if_snd);
rt2860_start(ifp);
@@ -1481,12 +1481,11 @@ rt2860_tx(struct rt2860_softc *sc, struc
struct rt2860_txd *txd;
struct rt2860_txwi *txwi;
struct ieee80211_frame *wh;
- struct mbuf *m1;
bus_dma_segment_t *seg;
u_int hdrlen;
uint16_t qos, dur;
uint8_t type, qsel, mcs, pid, tid, qid;
- int nsegs, ntxds, hasqos, ridx, ctl_ridx, error;
+ int nsegs, hasqos, ridx, ctl_ridx;
/* the data pool contains at least one element, pick the first */
data = SLIST_FIRST(&sc->data_pool);
@@ -1606,63 +1605,27 @@ rt2860_tx(struct rt2860_softc *sc, struc
memcpy(txwi + 1, wh, hdrlen);
m_adj(m, hdrlen);
- error = bus_dmamap_load_mbuf(sc->sc_dmat, data->map, m,
- BUS_DMA_NOWAIT);
- if (__predict_false(error != 0 && error != EFBIG)) {
- printf("%s: can't map mbuf (error %d)\n",
- sc->sc_dev.dv_xname, error);
- m_freem(m);
- return error;
- }
- if (__predict_true(error == 0)) {
- /* determine how many TXDs are required */
- ntxds = 1 + (data->map->dm_nsegs / 2);
-
- if (ring->queued + ntxds >= RT2860_TX_RING_MAX) {
- /* not enough free TXDs, force mbuf defrag */
- bus_dmamap_unload(sc->sc_dmat, data->map);
- error = EFBIG;
- }
- }
- if (__predict_false(error != 0)) {
- /* too many fragments, linearize */
- MGETHDR(m1, M_DONTWAIT, MT_DATA);
- if (m1 == NULL) {
- m_freem(m);
- return ENOBUFS;
- }
- if (m->m_pkthdr.len > MHLEN) {
- MCLGET(m1, M_DONTWAIT);
- if (!(m1->m_flags & M_EXT)) {
- m_freem(m);
- m_freem(m1);
- return ENOBUFS;
- }
- }
- m_copydata(m, 0, m->m_pkthdr.len, mtod(m1, caddr_t));
- m1->m_pkthdr.len = m1->m_len = m->m_pkthdr.len;
- m_freem(m);
- m = m1;
+ KASSERT (ring->queued <= RT2860_TX_RING_ONEMORE); /* <1> */
- error = bus_dmamap_load_mbuf(sc->sc_dmat, data->map, m,
- BUS_DMA_NOWAIT);
- if (__predict_false(error != 0)) {
- printf("%s: can't map mbuf (error %d)\n",
- sc->sc_dev.dv_xname, error);
- m_freem(m);
- return error;
- }
-
- /* determine how many TXDs are now required */
- ntxds = 1 + (data->map->dm_nsegs / 2);
-
- if (ring->queued + ntxds >= RT2860_TX_RING_MAX) {
- /* this is a hopeless case, drop the mbuf! */
- bus_dmamap_unload(sc->sc_dmat, data->map);
- m_freem(m);
- return ENOBUFS;
- }
- }
+ if (bus_dmamap_load_mbuf(sc->sc_dmat, data->map, m, BUS_DMA_NOWAIT)) {
+ if (m_defrag(m, M_DONTWAIT))
+ return (ENOBUFS);
+ if (bus_dmamap_load_mbuf(sc->sc_dmat,
+ data->map, m, BUS_DMA_NOWAIT))
+ return (EFBIG);
+ }
+
+ /* The map will fit into the tx ring: (a "full" ring may have a few
+ * unused descriptors, at most (txds(MAX_SCATTER) - 1))
+ *
+ * ring->queued + txds(data->map->nsegs)
+ * <= { <0> data->map->nsegs <= MAX_SCATTER }
+ * ring->queued + txds(MAX_SCATTER)
+ * <= { <1> ring->queued <= TX_RING_MAX - txds(MAX_SCATTER) }
+ * TX_RING_MAX - txds(MAX_SCATTER) + txds(MAX_SCATTER)
+ * <= { arithmetic }
+ * TX_RING_MAX
+ */
qsel = (qid < EDCA_NUM_AC) ? RT2860_TX_QSEL_EDCA : RT2860_TX_QSEL_MGMT;
@@ -1713,8 +1676,8 @@ rt2860_tx(struct rt2860_softc *sc, struc
qid, txwi->wcid, data->map->dm_nsegs, ridx));
ring->cur = (ring->cur + 1) % RT2860_TX_RING_COUNT;
- ring->queued += ntxds;
- if (ring->queued >= RT2860_TX_RING_MAX)
+ ring->queued += 1 + (data->map->dm_nsegs / 2);
+ if (ring->queued > RT2860_TX_RING_ONEMORE)
sc->qfullmsk |= 1 << qid;
/* kick Tx */
@@ -1771,6 +1734,7 @@ sendit:
bpf_mtap(ic->ic_rawbpf, m, BPF_DIRECTION_OUT);
#endif
if (rt2860_tx(sc, m, ni) != 0) {
+ m_freem(m);
ieee80211_release_node(ic, ni);
ifp->if_oerrors++;
continue;
Index: sys/dev/ic/rt2860var.h
===================================================================
--- sys.orig/dev/ic/rt2860var.h
+++ sys/dev/ic/rt2860var.h
@@ -17,13 +17,15 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
+#define RT2860_MAX_SCATTER 15
+#define RT2860_MAX_SCATTER_TXD (1 + (RT2860_MAX_SCATTER / 2))
+
#define RT2860_RX_RING_COUNT 128
#define RT2860_TX_RING_COUNT 64
#define RT2860_TX_RING_MAX (RT2860_TX_RING_COUNT - 1)
+#define RT2860_TX_RING_ONEMORE (RT2860_TX_RING_MAX - RT2860_MAX_SCATTER_TXD)
#define RT2860_TX_POOL_COUNT (RT2860_TX_RING_COUNT * 2)
-#define RT2860_MAX_SCATTER ((RT2860_TX_RING_COUNT * 2) - 1)
-
/* HW supports up to 255 STAs */
#define RT2860_WCID_MAX 254
#define RT2860_AID2WCID(aid) ((aid) & 0xff)
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic