svn commit: r277122 - projects/ifnet/sys/dev/msk

John Baldwin jhb at FreeBSD.org
Wed Jan 14 14:33:06 UTC 2015


On 1/13/15 6:17 PM, Gleb Smirnoff wrote:
>   John,
> 
> On Tue, Jan 13, 2015 at 01:22:09PM -0500, John Baldwin wrote:
> J> >   o Convert from xxx_start(ifp) to xxx_transmit(ifp, m).
> J> >     A simple conversion itself is quite straight:
> J> >     * In ifdriver declaration define .ifdrv_maxqlen, take the value
> J> >       from IFQ_SET_MAXLEN() macro.
> J> >     * In ifdriver ifops declaration define if_transmit function.
> J> >     * Rename xxx_start() to xxx_transmit() and change its prototype.
> J> >     * The new named xxx_transmit() should:
> J> >       - Try to if_snd_enqueue() the mbuf or return.
> J> >       - Try to mtx_lock() the driver softc or return.
> J> 
> J> Please no.  This is a major source of pain with the current drivers that use 
> J> buf_ring that is worked around in various (often broken) ways.  The problem
> J> is that the other thread holding the lock might drop it right after your try 
> J> lock fails and then the packet you just queued doesn't go out until you queue 
> J> another packet.  That's all fine and good if you are blasting packets out an 
> J> interface.  It is not fine and good if you have sparse traffic.  That packet 
> J> is now potentially delayed indefinitely.  (In a real world scenario with a 
> J> heartbeat protocol that sent out timing packets once a second or so to measure 
> J> RTT between hosts this manifested as odd timings because we would see the 
> J> timings jump by a second every so often when the packet was delayed until the 
> J> next heartbeat).
> 
> Yes, I also see this problem when coding that.
> 
> J> This kind of design decision is why I do not want to just blindly convert 
> J> everything to suboptimal if_transmit routines.  The current ifq based if_start 
> J> model is better for 10/100 single-queue drivers than this.
> 
> If I convert mtx_trylock() to mtx_lock(), then we will get 1:1 conversion
> to the current model. But, I still want to do an improvement while converting
> drivers. :)

The trylock isn't an improvement.  I also worry that this approach is
going to result in a lot of copy and pasted code in various drivers as
they all reimplement the current if_transmit() (effectively).

> May be the xxx_transmit() should check the queue again after softc unlock?

I posted some ideas about how to handle this in a thread several years
ago on net@ with various alternatives.  In that case I was focused on
buf_ring and I settled on an approach where a draining thread marked the
queue as "busy" while it was draining it and cleared that flag before
checking the head of the queue.  The enqueue process returned a
different errno value (EINPROGRESS or some such) if it queued a packet
into a "busy" queue and the transmit routines were changed to 1) always
enqueue the packet, and 2) if EINPROGRESS wasn't returned, use a
blocking mtx_lock and start transmitting.

However, even this model has some downsides in that one thread might be
stuck transmitting packets queued by other threads and never pop back
out to userland to service its associated application (a kind of
starvation of the user side of the thread).  Note that the mtx_trylock
approach has the same problem.  It might be nice to have a sort of limit
on the number of packets a thread is willing to enqueue, but then you
have the problem of ensuring any packets still on the queue when it hits
its limit aren't also delayed indefinitely.

I don't recall the exact mechanics of how Navdeep's mp_ring addresses
this (though I believe it does from when I looked at it).

Regardless, I think this was my point I attempted to make on IRC
earlier: you need to figure out what you are going to do here first
before you go through and convert all the drivers.  Otherwise you will
be stuck making multiple passes.  Converting a "real" driver up front is
useful so you can prototype different solutions, but I think you need to
resolve this now before continuing your pass as the current approach is
not suitable to be merged into HEAD.

-- 
John Baldwin


More information about the svn-src-projects mailing list