Date: Tue, 19 Nov 2013 11:04:57 +0000 (GMT) From: Robert Watson <rwatson@FreeBSD.org> To: "George V. Neville-Neil" <gnn@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r258328 - head/sys/net Message-ID: <alpine.BSF.2.00.1311191101060.50802@fledge.watson.org> In-Reply-To: <201311182258.rAIMwEFd048783@svn.freebsd.org> References: <201311182258.rAIMwEFd048783@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 18 Nov 2013, George V. Neville-Neil wrote:
> Allow ethernet drivers to pass in packets connected via the nextpkt pointer.
> Handling packets in this way allows drivers to amortize work during packet reception.
>
> Submitted by: Vijay Singh
> Sponsored by: NetApp
Currently, it is quite easy to make mistakes regarding individual mbuf chains
vs. lists of mbuf chains. This leads me to wonder whether a new type, perhaps
simply constructed on the stack before passing in, should be used for KPIs
that accept lists of packets. E.g.,
/*
* This structure is almost always allocated on a caller stack, so
* cannot itself be queued without memory allocation in most cases.
*/
struct mbuf_queue {
struct mbuf *mq_head;
};
int
ether_input(struct ifnet *ifp, struct mbuf_queue *m)
{
...
}
...
struct mbuf_queue mq = { m };
return (ether_input(ifp, &mq));
...
That way the compiler can help us figure out where we expect an individual
packet but have accidentally leaked a queue. Functions that accept only a
single packet could also more agressively assert that m->m_nextpkt is NULL:
M_ASSERT_ONEPACKET(m);
Robert
>
> Modified:
> head/sys/net/if_ethersubr.c
>
> Modified: head/sys/net/if_ethersubr.c
> ==============================================================================
> --- head/sys/net/if_ethersubr.c Mon Nov 18 22:55:50 2013 (r258327)
> +++ head/sys/net/if_ethersubr.c Mon Nov 18 22:58:14 2013 (r258328)
> @@ -708,13 +708,25 @@ static void
> ether_input(struct ifnet *ifp, struct mbuf *m)
> {
>
> + struct mbuf *mn;
> +
> /*
> - * We will rely on rcvif being set properly in the deferred context,
> - * so assert it is correct here.
> + * The drivers are allowed to pass in a chain of packets linked with
> + * m_nextpkt. We split them up into separate packets here and pass
> + * them up. This allows the drivers to amortize the receive lock.
> */
> - KASSERT(m->m_pkthdr.rcvif == ifp, ("%s: ifnet mismatch", __func__));
> + while (m) {
> + mn = m->m_nextpkt;
> + m->m_nextpkt = NULL;
>
> - netisr_dispatch(NETISR_ETHER, m);
> + /*
> + * We will rely on rcvif being set properly in the deferred context,
> + * so assert it is correct here.
> + */
> + KASSERT(m->m_pkthdr.rcvif == ifp, ("%s: ifnet mismatch", __func__));
> + netisr_dispatch(NETISR_ETHER, m);
> + m = mn;
> + }
> }
>
> /*
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1311191101060.50802>