Date: Tue, 1 Sep 2015 14:20:43 +0300 From: Gleb Smirnoff <glebius@FreeBSD.org> To: adrian@FreeBSD.org, Andriy Voskoboinyk <s3erios@gmail.com>, lstewart@FreeBSD.org Cc: net@FreeBSD.org Subject: mbufq-less iwn(4) Message-ID: <20150901112043.GB1023@glebius.int.ru>
next in thread | raw e-mail | index | archive | help
--wRRV7LY7NUeQGEoC
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Hi, Adrian and Andriy!
One of the fundamental things that me and Lawrence want to bring to
FreeBSD 12 (probably won't be in time with 11), is the NIC TX exhaustion
notification to the stack, also named as "network stack backpressure".
The problem is that when NICs TX queue is full we start to lose packets,
just as if they were lost somewhere in a wire, outside of our control.
Of course TCP engine copes with that and does necessary retransmitting.
The idea is that we can make TCP perform much better, is we report the
TX problem to it via ENOBUFS and DO NOT free the mbuf, since protocol
may have better idea on its destiny.
In the projects/ifnet branch, I already put this idea into the code.
In the branch drivers if_transmit doesn't free. I also put this idea
into the recent net80211 changes. New ic_transmit doesn't free. However,
due to most drivers have software queues as historical artifact, this
new semantic of ic_transmit is degenerated.
So, the long term plan is to slowly get rid of software mbuf queues
in drivers. The protocols should care about queueing (for example
ARP already does :)). We probably won't be able to get rid of software
queues everywhere before Lawrence does the needed work to TCP, since
now software queues are smoothing transmission for drivers that have
very small hardware queues.
Speaking particularly about iwn(4). Looks like the hardware has
enough descriptors to run w/o software queue. Right now writing
this email through WiFi connection with patched driver. Didn't
notice any issues with download/upload speed.
Any objections on checking it in?
--
Totus tuus, Glebius.
--wRRV7LY7NUeQGEoC
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="iwn-queueless.diff"
Index: if_iwn.c
===================================================================
--- if_iwn.c (revision 287348)
+++ if_iwn.c (working copy)
@@ -231,7 +231,6 @@ static void iwn_xmit_task(void *arg0, int pending)
static int iwn_raw_xmit(struct ieee80211_node *, struct mbuf *,
const struct ieee80211_bpf_params *);
static int iwn_transmit(struct ieee80211com *, struct mbuf *);
-static void iwn_start_locked(struct iwn_softc *);
static void iwn_watchdog(void *);
static int iwn_ioctl(struct ieee80211com *, u_long , void *);
static void iwn_parent(struct ieee80211com *);
@@ -471,7 +470,6 @@ iwn_attach(device_t dev)
}
IWN_LOCK_INIT(sc);
- mbufq_init(&sc->sc_snd, ifqmaxlen);
/* Read hardware revision and attach. */
sc->hw_type = (IWN_READ(sc, IWN_HW_REV) >> IWN_HW_REV_TYPE_SHIFT)
@@ -1409,8 +1407,6 @@ iwn_detach(device_t dev)
ieee80211_ifdetach(&sc->sc_ic);
}
- mbufq_drain(&sc->sc_snd);
-
/* Uninstall interrupt handler. */
if (sc->irq != NULL) {
bus_teardown_intr(dev, sc->irq, sc->sc_ih);
@@ -3597,14 +3593,10 @@ iwn_tx_done(struct iwn_softc *sc, struct iwn_rx_de
(status & IWN_TX_FAIL) != 0);
sc->sc_tx_timer = 0;
- if (--ring->queued < IWN_TX_RING_LOMARK) {
+ if (--ring->queued < IWN_TX_RING_LOMARK)
sc->qfullmsk &= ~(1 << ring->qid);
- if (sc->qfullmsk == 0)
- iwn_start_locked(sc);
- }
DPRINTF(sc, IWN_DEBUG_TRACE, "->%s: end\n",__func__);
-
}
/*
@@ -3781,14 +3773,10 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, int qid, i
}
sc->sc_tx_timer = 0;
- if (ring->queued < IWN_TX_RING_LOMARK) {
+ if (ring->queued < IWN_TX_RING_LOMARK)
sc->qfullmsk &= ~(1 << ring->qid);
- if (sc->qfullmsk == 0)
- iwn_start_locked(sc);
- }
DPRINTF(sc, IWN_DEBUG_TRACE, "->%s: end\n",__func__);
-
}
/*
@@ -4948,57 +4936,33 @@ iwn_raw_xmit(struct ieee80211_node *ni, struct mbu
static int
iwn_transmit(struct ieee80211com *ic, struct mbuf *m)
{
- struct iwn_softc *sc;
+ struct iwn_softc *sc = ic->ic_softc;
+ struct ieee80211_node *ni;
int error;
- sc = ic->ic_softc;
-
IWN_LOCK(sc);
- if ((sc->sc_flags & IWN_FLAG_RUNNING) == 0) {
+ if ((sc->sc_flags & IWN_FLAG_RUNNING) == 0 || sc->sc_beacon_wait) {
IWN_UNLOCK(sc);
return (ENXIO);
}
- error = mbufq_enqueue(&sc->sc_snd, m);
- if (error) {
+
+ if (sc->qfullmsk) {
IWN_UNLOCK(sc);
- return (error);
+ return (ENOBUFS);
}
- iwn_start_locked(sc);
+
+ ni = (struct ieee80211_node *)m->m_pkthdr.rcvif;
+ error = iwn_tx_data(sc, m, ni);
+ if (error) {
+ if_inc_counter(ni->ni_vap->iv_ifp, IFCOUNTER_OERRORS, 1);
+ ieee80211_free_node(ni);
+ } else
+ sc->sc_tx_timer = 5;
IWN_UNLOCK(sc);
- return (0);
+ return (error);
}
static void
-iwn_start_locked(struct iwn_softc *sc)
-{
- struct ieee80211_node *ni;
- struct mbuf *m;
-
- IWN_LOCK_ASSERT(sc);
-
- /*
- * If we're waiting for a beacon, we can just exit out here
- * and wait for the taskqueue to be kicked.
- */
- if (sc->sc_beacon_wait) {
- return;
- }
-
- DPRINTF(sc, IWN_DEBUG_XMIT, "%s: called\n", __func__);
- while (sc->qfullmsk == 0 &&
- (m = mbufq_dequeue(&sc->sc_snd)) != NULL) {
- ni = (struct ieee80211_node *)m->m_pkthdr.rcvif;
- if (iwn_tx_data(sc, m, ni) != 0) {
- if_inc_counter(ni->ni_vap->iv_ifp,
- IFCOUNTER_OERRORS, 1);
- ieee80211_free_node(ni);
- } else
- sc->sc_tx_timer = 5;
- }
- DPRINTF(sc, IWN_DEBUG_XMIT, "%s: done\n", __func__);
-}
-
-static void
iwn_watchdog(void *arg)
{
struct iwn_softc *sc = arg;
@@ -8731,9 +8695,6 @@ iwn_panicked(void *arg0, int pending)
"%s: could not move to run state\n", __func__);
}
- /* Only run start once the NIC is in a useful state, like associated */
- iwn_start_locked(sc);
-
IWN_UNLOCK(sc);
}
Index: if_iwnvar.h
===================================================================
--- if_iwnvar.h (revision 287348)
+++ if_iwnvar.h (working copy)
@@ -238,7 +238,6 @@ struct iwn_softc {
struct cdev *sc_cdev;
struct mtx sc_mtx;
struct ieee80211com sc_ic;
- struct mbufq sc_snd;
u_int sc_flags;
#define IWN_FLAG_HAS_OTPROM (1 << 1)
--wRRV7LY7NUeQGEoC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150901112043.GB1023>