From owner-svn-src-projects@FreeBSD.ORG Fri Mar 6 02:41:06 2015 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8BF5A91A; Fri, 6 Mar 2015 02:41:06 +0000 (UTC) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.69.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebius.int.ru", Issuer "cell.glebius.int.ru" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 10F59D9A; Fri, 6 Mar 2015 02:41:05 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.9/8.14.9) with ESMTP id t262f1e3083277 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 6 Mar 2015 05:41:01 +0300 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.9/8.14.9/Submit) id t262f13v083276; Fri, 6 Mar 2015 05:41:01 +0300 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Fri, 6 Mar 2015 05:41:01 +0300 From: Gleb Smirnoff To: John Baldwin Subject: Re: svn commit: r277122 - projects/ifnet/sys/dev/msk Message-ID: <20150306024101.GT17947@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> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="vni90+aGYgRvsTuO" Content-Disposition: inline In-Reply-To: <20150114144358.GD15484@FreeBSD.org> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Mar 2015 02:41:06 -0000 --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--