svn commit: r323942 - head/sys/net

[Bjoern A. Zeeb]

On 25 Sep 2017, at 17:04, Stephen Hurd wrote:

> Bjoern A. Zeeb wrote:
>> On 23 Sep 2017, at 23:46, Stephen Hurd wrote:
>>
>>> Bjoern A. Zeeb wrote:
>>>> On 23 Sep 2017, at 6:32, Stephen Hurd wrote:
>>>>
>>>>> Bjoern A. Zeeb wrote:
>>>>>> On 23 Sep 2017, at 1:35, Stephen Hurd wrote:
>>>>>>
>>>>>>> Author: shurd
>>>>>>> Date: Sat Sep 23 01:35:14 2017
>>>>>>> New Revision: 323942
>>>>>>> URL: https://svnweb.freebsd.org/changeset/base/323942
>>>>>>>
>>>>>>> Log:
>>>>>>>    Chain mbufs before passing to if_input()
>>>>>>>
>>>>>>>    Build a list of mbufs to pass to if_input() after LRO. 
>>>>>>> Results in
>>>>>>>    12% small packet forwarding rate improvement.

>> I not saying anything against the change, I am just saying the commit 
>> message doesn’t describe what it does.
>
> Can you explain what was confusing about it or propose other wording?  
> I'm not sure what was confusing, and I'd like to avoid similarly 
> confusing messages in the future.

I think it’s because I read “after LRO” as “after LRO processing 
happened” which is exactly not what is happening in that case;  I know 
logically in the code order it’s “after LRO”.

If I understand the change correctly (and I think jtl summarised it 
quite well already as well):

“In cases when LRO is disabled or LRO is not consuming the packet,
try to build an mbuf chain and pass the chain to if_input() thus 
lowering the per-packet overheads (*).
For a packet forwarding case we have seen a 12% rate improvement for 
small packets.”

(*) would be nice to describe them at this point so people understand 
where 12% come from (e.g., function call overhead, locking overhead, 
whatever ..) because that’s the reason you are doing the change.


>> Also I am pretty sure this works with ether_input but not so much 
>> with fddi_input, iso88025_input, and ifdead_input is probably going 
>> to leak as well.
>
> Thanks for the heads up.  They all seem to use m_freem(), so they 
> shouldn't leak.

Right.  My bad.

> It doesn't look like they would actually work though (except 
> ifdead_input of course).

Well, they’d work with a bit of packet loss I guess ;-)

/bz