Date: Fri, 6 Mar 2015 05:41:01 +0300 From: Gleb Smirnoff <glebius@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r277122 - projects/ifnet/sys/dev/msk Message-ID: <20150306024101.GT17947@FreeBSD.org> In-Reply-To: <20150114144358.GD15484@FreeBSD.org> References: <201501130902.t0D927NE077024@svn.freebsd.org> <5330876.Sb1U9Iz8Cz@ralph.baldwin.cx> <20150113231735.GZ15484@FreeBSD.org> <54B67E20.3090701@FreeBSD.org> <20150114144358.GD15484@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--vni90+aGYgRvsTuO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, Jan 14, 2015 at 05:43:58PM +0300, Gleb Smirnoff wrote:
T> On Wed, Jan 14, 2015 at 09:33:04AM -0500, John Baldwin wrote:
T> J> I posted some ideas about how to handle this in a thread several years
T> J> ago on net@ with various alternatives. In that case I was focused on
T> J> buf_ring and I settled on an approach where a draining thread marked the
T> J> queue as "busy" while it was draining it and cleared that flag before
T> J> checking the head of the queue. The enqueue process returned a
T> J> different errno value (EINPROGRESS or some such) if it queued a packet
T> J> into a "busy" queue and the transmit routines were changed to 1) always
T> J> enqueue the packet, and 2) if EINPROGRESS wasn't returned, use a
T> J> blocking mtx_lock and start transmitting.
T> J>
T> J> However, even this model has some downsides in that one thread might be
T> J> stuck transmitting packets queued by other threads and never pop back
T> J> out to userland to service its associated application (a kind of
T> J> starvation of the user side of the thread). Note that the mtx_trylock
T> J> approach has the same problem. It might be nice to have a sort of limit
T> J> on the number of packets a thread is willing to enqueue, but then you
T> J> have the problem of ensuring any packets still on the queue when it hits
T> J> its limit aren't also delayed indefinitely.
T>
T> Thanks, I will try to code that.
John, can you please look at this patch? It is against projects/ifnet.
The idea is that if_snd_enqueue() tells us whether we grabbed to queue and
own it or not. If we grabbed it, we go processing it to the end. However,
we keep accounting on how many packets we processed there. If other
producer notices that we processed too much, it will preempt the queue.
Looks like a design that matches your demands. However, extra code needs
to be put into drivers foo_start() functions, since now we need to disown
the queue if we stop processing it for some reason different to queue getting
empty.
--
Totus tuus, Glebius.
--vni90+aGYgRvsTuO
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="if_snd.diff"
Index: sys/net/if.c
===================================================================
--- sys/net/if.c (revision 279670)
+++ sys/net/if.c (working copy)
@@ -3538,12 +3538,21 @@ if_foreach_maddr(if_t ifp, ifmaddr_cb_t cb, void *
/*
* Generic software queue, that many non-high-end drivers use. For now
- * it is minimalistic version of classic BSD ifqueue, but we can swap it
- * to any other implementation later.
+ * it is based on classic BSD ifqueue, but we can swap it to any other
+ * implementation later.
*/
struct ifqueue {
+ struct mtx ifq_mtx;
struct mbufq ifq_mbq;
- struct mtx ifq_mtx;
+ /*
+ * The ifqueue is a very simple and apparently not scalable
+ * multiple producer / single consumer queue. Multiple threads
+ * put packets on the queue, and one of threads would dequeue
+ * and process them. Its burst is limited to queue length, to
+ * avoid starvation.
+ */
+ struct thread *ifq_owner;
+ int ifq_burst;
};
static struct ifqueue *
@@ -3554,6 +3563,8 @@ if_snd_alloc(int maxlen)
ifq = malloc(sizeof(struct ifqueue), M_IFNET, M_WAITOK);
mbufq_init(&ifq->ifq_mbq, maxlen);
mtx_init(&ifq->ifq_mtx, "ifqueue", NULL, MTX_DEF | MTX_NEW);
+ ifq->ifq_owner = NULL;
+ ifq->ifq_burst = 0;
return (ifq);
}
@@ -3595,11 +3606,21 @@ if_snd_enqueue(struct ifnet *ifp, struct mbuf *m)
mtx_lock(&ifq->ifq_mtx);
error = mbufq_enqueue(&ifq->ifq_mbq, m);
- mtx_unlock(&ifq->ifq_mtx);
if (error) {
+ mtx_unlock(&ifq->ifq_mtx);
m_freem(m);
if_inc_counter(ifp, IFCOUNTER_OQDROPS, 1);
+ return (error);
}
+ if (ifq->ifq_burst <= 0) {
+ /* Take ownership of the queue. */
+ KASSERT(ifq->ifq_owner != curthread, ("%s: %p is owner",
+ __func__, curthread));
+ ifq->ifq_owner = curthread;
+ ifq->ifq_burst = ifq->ifq_mbq.mq_maxlen;
+ } else
+ error = EBUSY;
+ mtx_unlock(&ifq->ifq_mtx);
return (error);
}
@@ -3610,22 +3631,58 @@ if_snd_dequeue(if_t ifp)
struct mbuf *m;
mtx_lock(&ifq->ifq_mtx);
+ if (ifq->ifq_owner != curthread) {
+ if (ifq->ifq_owner == 0) {
+ ifq->ifq_owner = curthread;
+ ifq->ifq_burst = ifq->ifq_mbq.mq_maxlen;
+ } else {
+ /* Other thread preempted us. */
+ mtx_unlock(&ifq->ifq_mtx);
+ return (NULL);
+ }
+ }
m = mbufq_dequeue(&ifq->ifq_mbq);
+ if (m == NULL) {
+ ifq->ifq_owner = NULL;
+ ifq->ifq_burst = 0;
+ } else
+ ifq->ifq_burst--;
mtx_unlock(&ifq->ifq_mtx);
return (m);
}
void
-if_snd_prepend(if_t ifp, struct mbuf *m)
+if_snd_unown(if_t ifp)
{
struct ifqueue *ifq = ifp->if_snd;
mtx_lock(&ifq->ifq_mtx);
- mbufq_prepend(&ifq->ifq_mbq, m);
+ if (ifq->ifq_owner == curthread) {
+ ifq->ifq_owner = NULL;
+ ifq->ifq_burst = 0;
+ }
mtx_unlock(&ifq->ifq_mtx);
}
/*
+ * Put back dequeued mbuf (which can be NULL), and unown the queue.
+ */
+void
+if_snd_putback(if_t ifp, struct mbuf *m)
+{
+ struct ifqueue *ifq = ifp->if_snd;
+
+ mtx_lock(&ifq->ifq_mtx);
+ if (m != NULL)
+ mbufq_prepend(&ifq->ifq_mbq, m);
+ if (ifq->ifq_owner == curthread) {
+ ifq->ifq_owner = NULL;
+ ifq->ifq_burst = 0;
+ }
+ mtx_unlock(&ifq->ifq_mtx);
+}
+
+/*
* Implementation of if ops, that can be called from drivers.
*/
void
Index: sys/net/if.h
===================================================================
--- sys/net/if.h (revision 279669)
+++ sys/net/if.h (working copy)
@@ -740,7 +740,8 @@ void if_foreach_maddr(if_t, ifmaddr_cb_t, void *);
int if_snd_len(if_t);
int if_snd_enqueue(if_t, struct mbuf *);
struct mbuf * if_snd_dequeue(if_t);
-void if_snd_prepend(if_t, struct mbuf *);
+void if_snd_unown(if_t);
+void if_snd_putback(if_t, struct mbuf *);
/*
* Type-enforcing inliners over if_getsoftc().
Index: sys/dev/msk/if_msk.c
===================================================================
--- sys/dev/msk/if_msk.c (revision 279669)
+++ sys/dev/msk/if_msk.c (working copy)
@@ -2890,12 +2890,14 @@ msk_transmit(if_t ifp, struct mbuf *m)
struct msk_if_softc *sc_if;
int error;
- if ((error = if_snd_enqueue(ifp, m)) != 0)
+ error = if_snd_enqueue(ifp, m);
+ if (error == EBUSY)
+ return (0);
+ else if (error)
return (error);
sc_if = if_getsoftc(ifp, IF_DRIVER_SOFTC);
- if (MSK_IF_TRYLOCK(sc_if) == 0)
- return (0);
+ MSK_IF_LOCK(sc_if);
error = msk_start(sc_if);
MSK_IF_UNLOCK(sc_if);
return (error);
@@ -2909,17 +2911,22 @@ msk_start(struct msk_if_softc *sc_if)
MSK_IF_LOCK_ASSERT(sc_if);
- if ((sc_if->msk_flags & MSK_FLAG_LINK) == 0)
+ if ((sc_if->msk_flags & MSK_FLAG_LINK) == 0) {
+ if_snd_unown(sc_if->msk_ifp);
return (ENETDOWN);
+ }
error = enq = 0;
- while (sc_if->msk_cdata.msk_tx_cnt <
- (MSK_TX_RING_CNT - MSK_RESERVED_TX_DESC_CNT) &&
- (m = if_snd_dequeue(sc_if->msk_ifp)) != NULL) {
+ for (;;) {
+ if (sc_if->msk_cdata.msk_tx_cnt >=
+ MSK_TX_RING_CNT - MSK_RESERVED_TX_DESC_CNT) {
+ if_snd_unown(sc_if->msk_ifp);
+ break;
+ }
+ if ((m = if_snd_dequeue(sc_if->msk_ifp)) == NULL)
+ break;
if ((error = msk_encap(sc_if, &m)) != 0) {
- if (m == NULL)
- break;
- if_snd_prepend(sc_if->msk_ifp, m);
+ if_snd_putback(sc_if->msk_ifp, m);
break;
}
enq++;
--vni90+aGYgRvsTuO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150306024101.GT17947>