svn commit: r301217 - in head/sys: net netinet netinet6

[Alexander V. Chernikov]

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.
>
> Best,
> George