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

[Qing Li]

On Sun, Jun 5, 2016 at 11:06 PM, Alexander V. Chernikov <
melifaro at freebsd.org> wrote:

> 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
>
> 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