-----Original Message----- From: owner-src-committ...@freebsd.org [mailto:owner-src-committ...@freebsd.org] On Behalf Of Alexander V. Chernikov Sent: Sunday, June 05, 2016 11:07 PM To: George Neville-Neil <g...@freebsd.org> Cc: Mike Karels <m...@karels.net>; src-committers <src-committ...@freebsd.org>; svn-src-all <svn-src-...@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" <g...@freebsd.org>: > On 4 Jun 2016, at 15:05, Alexander V. Chernikov wrote: > >> 02.06.2016, 20:51, "George V. Neville-Neil" <g...@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 _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"