Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 6 Jun 2016 09:32:08 -0700
From:      "Qing" <tomelite82@gmail.com>
To:        <melifaro@freebsd.org>, <gnn@freebsd.org>
Cc:        <mike@karels.net>, <src-committers@freebsd.org>, <svn-src-all@freebsd.org>, <svn-src-head@freebsd.org>
Subject:   RE: svn commit: r301217 - in head/sys: net netinet netinet6
Message-ID:  <000001d1c010$f8d603c0$ea820b40$@gmail.com>
In-Reply-To: <3505271465193212@web29h.yandex.ru>
References:  <201606021751.u52HpTrH090384@repo.freebsd.org>	 <3448221465067132@web17h.yandex.ru> <B3E52539-01F4-480C-9196-F9EF197E4887@freebsd.org> <3505271465193212@web29h.yandex.ru>

Next in thread | Previous in thread | Raw E-Mail | Index | Archive | Help

-----Original Message-----
From: owner-src-committers@freebsd.org =
[mailto:owner-src-committers@freebsd.org] On Behalf Of Alexander V. =
Chernikov
Sent: Sunday, June 05, 2016 11:07 PM
To: George Neville-Neil <gnn@freebsd.org>
Cc: Mike Karels <mike@karels.net>; src-committers =
<src-committers@freebsd.org>; svn-src-all <svn-src-all@freebsd.org>; =
svn-src-head <svn-src-head@freebsd.org>
Subject: Re: svn commit: r301217 - in head/sys: net netinet netinet6

06.06.2016, 04:40, "George Neville-Neil" <gnn@freebsd.org>:
> On 4 Jun 2016, at 15:05, Alexander V. Chernikov wrote:
>
>>  02.06.2016, 20:51, "George V. Neville-Neil" <gnn@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 =E2=80=9Cprepend=E2=80=9D 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 =
=E2=80=9Cro_prepend=E2=80=9D /
>>  ro_plen fields.
>>
>>  In general, this change looks more like a local hack and not like=20
>> 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=20
>> 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 =E2=80=9Cnetwork=E2=80=9D group was not included to the review =
despite the
>>  fact that most of the changes were related to the L2 layer which is
>>  not =E2=80=9Ctransport=E2=80=9D, 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 =E2=80=9Cplle=E2=80=9D argument is filled only in =
=E2=80=9Cheavy=E2=80=9D
>>  lltable lookup functions (e.g. when we don=E2=80=99t have neighbour
>>  adjacency). 99.9% of the time arpresolve/nd6_resolve() returns the
>>  result w/o calling their heavy versions, and the returned =
=E2=80=9Cplle=E2=80=9D
>>  is NULL.
>>
>>  This can be easily verified by calling something like
>>  dtrace -n 'fbt:kernel:ether_output:entry /arg3!=3DNULL&&((struct =
route
>>  *)arg3)->ro_lle !=3D 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=20
> Karels, from whom this originated.

>>No, please keep the discussion open. The decision on having that =
particular L2 caching=20
>>implementation (and L2 caching in general) is quite important, so it =
would be great if=20
>>all technical arguments were saved so other people can=20
>>(now or later) understand the decision details.
>>Thanks for understanding.

This commit does seem to undo the non-trivial layer separation efforts =
invested previously.=20
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







Want to link to this message? Use this URL: <http://docs.FreeBSD.org/cgi/mid.cgi?000001d1c010$f8d603c0$ea820b40$>