svn commit: r301217 - in head/sys: net netinet netinet6
Qing
tomelite82 at gmail.com
Mon Jun 6 16:32:11 UTC 2016
-----Original Message-----
From: owner-src-committers at freebsd.org [mailto:owner-src-committers at freebsd.org] On Behalf Of Alexander V. Chernikov
Sent: Sunday, June 05, 2016 11:07 PM
To: George Neville-Neil <gnn at freebsd.org>
Cc: Mike Karels <mike at karels.net>; src-committers <src-committers at freebsd.org>; svn-src-all <svn-src-all at freebsd.org>; svn-src-head <svn-src-head at freebsd.org>
Subject: Re: svn commit: r301217 - in head/sys: net netinet netinet6
06.06.2016, 04:40, "George Neville-Neil" <gnn at freebsd.org>:
> On 4 Jun 2016, at 15:05, Alexander V. Chernikov wrote:
>
>> 02.06.2016, 20:51, "George V. Neville-Neil" <gnn at freebsd.org>:
>>> Author: gnn
>>> Date: Thu Jun 2 17:51:29 2016
>>> New Revision: 301217
>>> URL: https://svnweb.freebsd.org/changeset/base/301217
>>>
>>> Log:
>>> This change re-adds L2 caching for TCP and UDP, as originally
>>> added in D4306
>>> but removed due to other changes in the system. Restore the
>>> llentry pointer
>>> to the "struct route", and use it to cache the L2 lookup (ARP or
>>> ND6) as
>>> appropriate.
>>
>> I have several comments regarding this commit.
>>
>> 1 Architecturally, there was quite a lot of efforts to eliminate
>> layering violation between lltable and other places in network stack.
>> It ended by committing D4102, which allowed both to cleanup lower
>> level, provide abstract “prepend” framework which could be used to
>> provide cached data to _otuput() functions.
>> This change brings these violations back in a really invasive way.
>>
>> Additionally, implementing L2 PCB caching at the other subsystem
>> expense is really a bad idea.
>> If one wants caching in some subsystem, it should be implemented in
>> that subsystem not polluting other things.
>> Current implementation permits this by filling in “ro_prepend” /
>> ro_plen fields.
>>
>> In general, this change looks more like a local hack and not like
>> the
>> code that should be included in the tree.
>>
>> 2 There was no benchmarks proving the effectiveness of this change.
>> (For example, it is not obvious if it could significantly improve
>> TCP
>> since we still have per-session TCP wlock + (typically) per-ring
>> mutex, so removing lltable rock might not help things here). Given
>> that the patch complicates existing code, there should be adequate
>> benefits to consider whether this change is worth implementing.
>>
>> 3 The “network” group was not included to the review despite the
>> fact that most of the changes were related to the L2 layer which is
>> not “transport”, so some people might have missed this review.
>>
>> 4 This change DOES NOT WORK. really.
>> (which raises questions on both review and benchmarking process).
>>
>> The reason is that “plle” argument is filled only in “heavy”
>> lltable lookup functions (e.g. when we don’t have neighbour
>> adjacency). 99.9% of the time arpresolve/nd6_resolve() returns the
>> result w/o calling their heavy versions, and the returned “plle”
>> is NULL.
>>
>> This can be easily verified by calling something like
>> dtrace -n 'fbt:kernel:ether_output:entry /arg3!=NULL&&((struct route
>> *)arg3)->ro_lle != NULL/ { stack(); }'
>>
>> Given that, I kindly ask you to backout this change.
>
> Hi,
>
> I'm going to limit the scope of this reply to just you, me and Mike
> Karels, from whom this originated.
>>No, please keep the discussion open. The decision on having that particular L2 caching
>>implementation (and L2 caching in general) is quite important, so it would be great if
>>all technical arguments were saved so other people can
>>(now or later) understand the decision details.
>>Thanks for understanding.
This commit does seem to undo the non-trivial layer separation efforts invested previously.
The flow-table construction was meant to help accelerate TCP/UDP route lookups. The various
aspects of the routing code took flow-table into consideration, and those code are still present
even after this change.
--Qing
More information about the svn-src-head
mailing list