svn commit: r194012 - in head: . sys/netgraph sys/sys

[Scott Long]

Julian Elischer wrote:
> Alexander Kabaev wrote:
>> On Thu, 11 Jun 2009 13:27:12 -0700
>> Julian Elischer <julian at elischer.org> wrote:
>>
>>> Marko Zec wrote:
>>>> On Thursday 11 June 2009 21:01:40 Pawel Jakub Dawidek wrote:
>>>>> On Thu, Jun 11, 2009 at 04:50:49PM +0000, Marko Zec wrote:
>>>>>> Author: zec
>>>>>> Date: Thu Jun 11 16:50:49 2009
>>>>>> New Revision: 194012
>>>>>> URL: http://svn.freebsd.org/changeset/base/194012
>>>>>>
>>>>>> Log:
>>>>>>   Introduce a mechanism for detecting calls from outbound path of
>>>>>> the network stack when reentering the inbound path from netgraph,
>>>>>> and force queueing of mbufs at the outbound netgraph node.
>>>>>>
>>>>>>   The mechanism relies on two components.  First, in netgraph
>>>>>> nodes where outbound path of the network stack calls into
>>>>>> netgraph, the current thread has to be appropriately marked using
>>>>>> the new NG_OUTBOUND_THREAD_REF() macro before proceeding to call
>>>>>> further into the netgraph topology, and unmarked using the
>>>>>>   NG_OUTBOUND_THREAD_UNREF() macro before returning to the caller.
>>>>>>   Second, netgraph nodes which can potentially reenter the network
>>>>>>   stack in the inbound path have to mark their inbound hooks using
>>>>>>   NG_HOOK_SET_TO_INBOUND() macro.  The netgraph framework will
>>>>>> then detect when there is a danger of a call graph looping back
>>>>>> from outbound to inbound path via netgraph, and defer handing off
>>>>>> the mbufs to the "inbound" node to a worker thread with a clean
>>>>>> stack.
>>>>>>
>>>>>>   In this first pass only the most obvious netgraph nodes have
>>>>>> been updated to ensure no outbound to inbound calls can occur.
>>>>>> Nodes such as ng_ipfw, ng_gif etc. should be further examined
>>>>>> whether a potential for outbound to inbound call looping exists.
>>>>>>
>>>>>>   This commit changes the layout of struct thread, but due to
>>>>>>   __FreeBSD_version number shortage a version bump has been
>>>>>> omitted at this time, nevertheless kernel and modules have to be
>>>>>> rebuilt.
>>>>> Are you sure Marko that you can't use sys/sys/osd.h instead of
>>>>> adding yet another field to the thread structure? Netgraph is
>>>>> optional component and optional components could take advantage of
>>>>> allocating stuff they need dynamically. The OSD (Object-Specific
>>>>> Data) KPI is designed for use by optional components - you can add
>>>>> your data to a thread, you can get it when you want and OSD will
>>>>> call your callback when thread dies, so you can clean up.
>>>>>
>>>>> Maybe you can't, but it's worth checking.
>>>> Hmm how much locking overhead do osd_set() / osd_get() methods
>>>> introduce?  We have to bump the refcount on each entry to netgraph,
>>>> and then check it potentially on each hop to next ng node, and
>>>> finally drop the refcount when done with the function call into
>>>> netgraph.  Accessing td_ng_outbound directly via curthread is as
>>>> cheap as it gets performancewise as it requires no locking
>>>> whatsoever...
>>> I would add that I suspect that we may end up using it in other
>>> places as well outside of netgraph.
>>>
>>
>> When that happens then per-thread field can be revisited again. Blowing
>> the side of major kernel structure for the sake of subsystem is
>> unused by 90%+ percent of users is little too drastic IMHO.
>>
>> I do second Pawel's opinion that you should look at osd for the time
>> being. After all it was invented for just this reason.
> 
> And I beg to dissagree.
> 
> Firstly this is not the first field to be put in these structures for a 
> single module.
> 
> Secondly, the overhead of doing it in the manner suggested would
> be quite noticeable I think, certainly a drain on what could be
> a fast-path for some packet processing.
> 
> 

You've already had the author of the osd code say that osd is designed 
to have little overhead.  Maybe it's time to at least give it a try and 
make some measurements?

Scott