Date: Wed, 11 Mar 2015 13:07:29 -0400 From: John Baldwin <jhb@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r277122 - projects/ifnet/sys/dev/msk Message-ID: <1823344.ReAEVtmWLW@ralph.baldwin.cx> In-Reply-To: <20150311151735.GE17947@FreeBSD.org> References: <201501130902.t0D927NE077024@svn.freebsd.org> <44632162.sBlScnaX0r@ralph.baldwin.cx> <20150311151735.GE17947@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, March 11, 2015 06:17:35 PM Gleb Smirnoff wrote:
> On Wed, Mar 11, 2015 at 10:08:30AM -0400, John Baldwin wrote:
> J> > John, can you please look at this patch? It is against projects/ifnet.
> J> >
> J> > The idea is that if_snd_enqueue() tells us whether we grabbed to queue and
> J> > own it or not. If we grabbed it, we go processing it to the end. However,
> J> > we keep accounting on how many packets we processed there. If other
> J> > producer notices that we processed too much, it will preempt the queue.
> J> >
> J> > Looks like a design that matches your demands. However, extra code needs
> J> > to be put into drivers foo_start() functions, since now we need to disown
> J> > the queue if we stop processing it for some reason different to queue getting
> J> > empty.
> J>
> J> I think this patch is not a bad approach. It resembles the last thing I
> J> posted to net@ except that you have added the burst length cap which is a
> J> nice addition. Of course, this uses a lock to do so which buf_ring tries to
> J> avoid.
> J>
> J> (I also eventually would love to have a way to move the enqueue out of drivers
> J> entirely still where there is a callback for "drain this queue" that only
> J> gets called in the !EBUSY case. I can't recall if that is compatible with your
> J> stacking approach, but it would make it harder for drivers to get this wrong
> J> if we can avoid duplicating it N times.)
>
> I really wish to avoid duplicating code in regular drivers foo_transmit
> and foo_start, but this doesn't seem easy for me.
>
> Typical foo_transmit looks like:
>
>
> if ((error = if_snd_enqueue(ifp, m)) != 0)
> return (error);
^^^^^^
It's this part that it would be nice to not duplicate. With buf_ring that
became quite a bit more complicated than just one line. I'm not sure yet
how you are intending to handle buf_ring, but ideally I'd like the stack to
know about transmit rings and to do this enqueuing (and the EBUSY check
your patch would add) outside of the driver, and only call down to
the driver when there is work to do for a queue. The remainder below this
is actually very close to the old if_start method.
>
> * LOCK(sc);
> * if (sc->link_not_active || sc->resources_not_allocated)
> return;
> * while (sc->tx_count <= DRIVER_CONST &&
> (m = if_snd_dequeue(ifp)) != NULL) {
> * if (driver_encap(sc, m, ...) != 0) {
> if_snd_prepend(ifp, m);
> break;
> }
> count++
> }
> if (count) {
> * /* TX unit programming */
> }
> * UNLOCK(sc);
>
> All lines marked with * are somewhat unique in drivers. So, cycle looks
> a like, but always differ in details. Right now I don't have ideas how to
> collapse that to a generic method. If anyone has, please share! :)
>
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1823344.ReAEVtmWLW>