svn commit: r254520 - in head/sys: kern sys

[Andre Oppermann]

On 19.08.2013 23:45, Navdeep Parhar wrote:
> On 08/19/13 13:58, Andre Oppermann wrote:
>> On 19.08.2013 19:33, Navdeep Parhar wrote:
>>> On 08/19/13 04:16, Andre Oppermann wrote:
>>>> Author: andre
>>>> Date: Mon Aug 19 11:16:53 2013
>>>> New Revision: 254520
>>>> URL: http://svnweb.freebsd.org/changeset/base/254520
>>>>
>>>> Log:
>>>>     Remove the unused M_NOFREE mbuf flag.  It didn't have any in-tree
>>>> users
>>>>     for a very long time, if ever.
>>>>
>>>>     Should such a functionality ever be needed again the appropriate and
>>>>     much better way to do it is through a custom EXT_SOMETHING
>>>> external mbuf
>>>>     type together with a dedicated *ext_free function.
>>>>
>>>>     Discussed with:    trociny, glebius
>>>>
>>>> Modified:
>>>>     head/sys/kern/kern_mbuf.c
>>>>     head/sys/kern/uipc_mbuf.c
>>>>     head/sys/sys/mbuf.h
>>>>
>>>
>>> Hello Andre,
>>>
>>> Is this just garbage collection or is there some other reason for this?
>>
>> This is garbage collection and removal of not quite right, rotten,
>> functionality.
>>
>>> I recently tried some experiments to reduce the number of mbuf and
>>> cluster allocations in a 40G NIC driver.  M_NOFREE and EXT_EXTREF proved
>>> very useful and the code changes to the kernel were minimal.  See
>>> user/np/cxl_tuning.  The experiment was quite successful and I was
>>> planning to bring in most of those changes to HEAD.  I was hoping to get
>>> some runtime mileage on the approach in general before tweaking the
>>> ctors/dtors for jumpbo, jumbo9, jumbo16 to allow for an mbuf+refcnt
>>> within the cluster.  But now M_NOFREE has vanished without a warning...
>>
>> I'm looking through your experimental code and that is some really good
>> numbers you're achieving there!
>>
>> However a couple things don't feel quite right, hackish even, and not
>> fit for HEAD.  This is a bit the same situation we had with some of the
>> first 1GigE cards quite a number of years back (mostly ti(4)).  There
>> we ended up with a couple of just good enough hacks to make it fast.
>> Most of the remains I've collected today.
>
> If M_NOFREE and EXT_EXTREF are properly supported in the tree (and I'm
> arguing that they were, before r254520) then the changes are perfectly
> legitimate.  The only hackish part was that I was getting the cluster
> from the jumbop zone while bypassing its normal refcnt mechanism.  This
> I did so as to use the same zone as m_uiotombuf to keep it "hot" for all
> consumers (driver + network stack).

If you insist I'll revert the commit removing M_NOFREE.  EXT_EXTREF isn't
touched yet, but should get better support.

The hackish part for me is that the driver again manages its own memory
pool.  Windows works that way, NetBSD is moving towards it while FreeBSD
has and remains at a central network memory pool.  The latter (our current)
way of doing it seems more efficient overall especially on heavily loaded
networked machines.  There may be significant queues building (think app
blocked having many sockets buffer fill up) up delaying the freeing and
returning of network memory resources.  Together with fragmentation this
can lead to bad very outcomes.  Router applications with many interfaces
also greatly benefit from central memory pools.

So I'm really not sure that we should move back in the driver owned pool
direction with lots of code duplication and copy-pasting (see NetBSD).
Also it is kinda weird to have a kernel based pool for data going down
the stack and another one in each driver for those going up.

Actually I'm of the opinion that we should stay with the central memory
pool and fix so that it works just as well for those cases a driver pool
currently performs better.

>> I believe most of the improvements you've shown can be implemented in
>> a more generic and safe way into the mbuf system.  Also a number of things
>> in your experimental code may have side-effects in situations other than
>> netperf runs.
>
> Agreed.  As I mentioned my long term plan was to tweak the jumboX zones
> to allow them to allocate cluster with embedded mbuf + refcount.  The
> M_NOFREE+EXT_EXTREF approach is the perfect bridge from here to there.
> It is non-intrusive and lends itself well to experimentation.

Agreed, full support on fixing the refcount issue.

>> To summarize what I get from your experimental branch commits:
>> - the Chelsio T4/T5 card can DMA multiple ethernet frames (packets) into
>>    a single memory buffer, provided it is large enough.
>> - you make use of that feature and point multiple m_ext mbufs into that
>>    buffer to separate the packets and send them up the stack.
>> - you embed the m_ext refcount into the single memory buffer as well.
>
> yes, yes, and yes.
>
>> - you recycle mbufs? (I'm not entirely clear on that as I'm not familiar
>>    with the cxgbe code)
>
> I recycle the cluster (and the embedded mbuf in it) when possible.  All
> depends on whether it's still in use by the time the rx queue needs it back.

There's always a couple of problems with driver managed pools.  Driver shutdown/
unloading requires all such managed mbufs to have returned before it can proceed.
This may take a undetermined long time as mbufs are sitting in socket buffers or
other queues.

>> Lets examine and discuss these parts:
>> - M_NOFREE wasn't really safe with bpf anyway at least for packets going
>>    down the stack.
>
> I see.  Can you point out the parts of bpf unable to deal with M_NOFREE?

Sorry, false alarm.

>> - Instead of M_NOFREE a custom *ext_free should be used that has the same
>>    and even more functionality.
>
> Yes, that's what I was planning too, with the jumboX zone changes.  It
> would be faster than running the m_ext's free function (which is a
> function dereference+call).

It would be a bit faster but how do you know when an mbuf itself has been
free'd and can be reused without notification?

>> - Recycling mbufs may cause imbalances to the mbuf pool with multiple cores
>>    and possibly NUMA in the future.  Not that we are very good with it at
>>    the moment but bypassing the mbuf allocator shouldn't become the norm.
>>    If it is a problem/bottleneck again it should be fixed, not worked around
>>    and copy-pasted n-times in so many drivers.
>
> If/when a cluster is recycled, it is given back to the same rx ithread
> that originally allocated it, and not not any other queue.  If the
> ithread stays in the same NUMA domain (and it really should) then
> recycling the cluster in the same queue really shouldn't cause imbalances.

OK. (Side note at the moment the mbuf allocator isn't NUMA aware at all.)

>> - jumbo9 and jumbo16 mbufs should not be used because they are more special
>>    than necessary with being KVM and physically contiguous.  This probably
>>    isn't necessary for the T4/T5 cards and any other modern DMA engine.
>>    Under heavy diverse network the kernel memory becomes fragmented and
>> can't
>>    find memory fulfilling both criteria anymore.  In fact both are an
>> artifact
>>    of the early GigE hacks when high speed DMA engines were in their
>> infancy.
>>    Both jumbo9 and jumbo16 should go away without direct replacement.
>
> The devices I deal will be able to cope, but I suspect this will be
> disruptive (for not enough reason to justify the disruption, imho) to
> others.  That's a separate discussion anyway.

Again using the jumbo9 or jumbo16 doesn't scale.  Wollman had it fail quite
badly on his heavily loaded NFS server.

The way to go should be 4K clusters as they are native to the architecture.
IIRC a PCIe DMA can't cross a 4K boundary anyway, so it having a number of
them together shouldn't have much of an impact except for consuming descriptors?

On a well loaded system it's almost impossible to obtain physically contiguous
pages after some time, except through pre-allocation.  But then the pre-allocation
has to be properly sized in advance and takes memory away from other uses.

>>    In your T4/T5 case the alternative would be either to a) allocate your
>>    own memory directly from KVM with the necessary properties (KVM and/or
>> phys
>>    contig); b) have such a generic kernel mbuf type fulfilling the same
>> role.
>>    There may be some cache line issues on non-x86 systems that have to be
>>    though and taken care of.
>> - Refcounts should have the option of being separately allocated.  It was
>>    mandatory to use the refcount zone so far because there wasn't any other
>>    type of refcount.  Now that we have one we should revisit the issue.
>>    Actually the entire mbuf allocation and initialization could be
>> streamlined
>>    as a whole.
>>
>> On a side note a different mbuf handling for the RX DMA rings may give some
>> significant improvements as well: allocate just a cluster without a mbuf
>> through
>> m_cljget() and put it into the RX ring.  Only when the DMA has put a
>> packet into
>> it allocated attach the mbuf (in the drivers RX function).  This avoids
>> the cache
>> pollution from touching the mbuf fields during initial allocation,
>> including the
>> refcount.
>
> Er, this is pretty much how cxgbe(4) has always done it.  A cluster is
> allocated with m_cljget, an mbuf is allocated with MT_NOINIT to avoid
> touching any line in the mbuf.  m_init is called later in the rx ithread.

Excellent.  I haven't (yet) spent much time reading the cxgbe(4) code.

>> Another nice trick would be to shorten the mbuf by 4 bytes (in ext_size)
>> and put
>> the refcount there.
>>
>> Lets work on these together.
>
> Yes.  I think we agree on the long term optimization, or are at least
> headed in the same general direction.  But removing M_NOFREE removed a
> dirt-cheap way to test and exercise the approach without implementing
> the "real thing."  Why not just let it be?

I don't mind too much having M_NOFREE return but, while convenient, I'm
not convinced at all that it is the right and best way of handling that
use case.

-- 
Andre