From owner-svn-src-head@freebsd.org Mon Jun 6 16:32:11 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 78593B6B942; Mon, 6 Jun 2016 16:32:11 +0000 (UTC) (envelope-from tomelite82@gmail.com) Received: from mail-pf0-x241.google.com (mail-pf0-x241.google.com [IPv6:2607:f8b0:400e:c00::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 484141CA1; Mon, 6 Jun 2016 16:32:11 +0000 (UTC) (envelope-from tomelite82@gmail.com) Received: by mail-pf0-x241.google.com with SMTP id u67so2185481pfu.0; Mon, 06 Jun 2016 09:32:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:references:in-reply-to:subject:date:message-id :mime-version:content-transfer-encoding:thread-index :content-language; bh=kzomo431CIaqSCCWFywFDwRFmtyt3c7O671X9Rnqb30=; b=uUVTFlhS4LcVTBml8jMElNvRUZ2jAJK4pSsejvN5VpQkLQndQUXRjOJ9f3aj6NkpLt WZZWrZ6ut+Kxvm3g4XO8Vwl5TX2Z0rdgzmMDfPLlLNXZzLuXFPK/goxk0gr6Y3rqDyrw 0gBhKUIuMsw8iAA5fls/IY4+cAInmY9WUA1W54mPl2iE7sj5ug++PklQsx+5HMRJOiSd S5EgfJUWLp443mpC5X07je7OyxXsa5oKZnIfGG1tUgxlWo/FQ7a4xazLba1iF3lLgGOW IVwSTwpTrIqDCBOEui0u2wo/x6kFw1jMQbDnSA+LbSA4Du3sGulz3Qvg5DzYVi7T83Bz LVqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:references:in-reply-to:subject:date :message-id:mime-version:content-transfer-encoding:thread-index :content-language; bh=kzomo431CIaqSCCWFywFDwRFmtyt3c7O671X9Rnqb30=; b=dsaGvefl2S5XL3QX4qTsOAyJIzr898vc6nmd2ERiDFutmVRcMHe3sdiKUbwHKLpoZ0 mQccFXky15GE4ZIt96j26yXtySLK4w1YbMqNDQ9EwSQx1UgDrm2tFecqcPWgDqHdNnq8 pAIZpTQUk7Ft/5Gp0YTP89q1piu5lBQDDtM1lxsMbQeLDiEZPljJyEuGKU9V1+XUKkkD 9HcKUNF49iSCVF1Vo2b8HOYca3fBkqNmI97w5v6ttPAGuw0kRwXLIiyTtKONEDa++i6N hbcy3kSDMGR41IGLZ+euzDrXZ/AeopcwBeaug93oqMMQ1h1nsL7x3FfVCUASoKYnq8BO kR9w== X-Gm-Message-State: ALyK8tKMoSNcL9M3ltm85RVxfPQPJ8Zks+6k6mciB2C8VBlhoV2i9hxApKlZWrp6bnOfjA== X-Received: by 10.98.65.209 with SMTP id g78mr26041428pfd.163.1465230730878; Mon, 06 Jun 2016 09:32:10 -0700 (PDT) Received: from Spirit (dsl081-051-141.sfo1.dsl.speakeasy.net. [64.81.51.141]) by smtp.gmail.com with ESMTPSA id c16sm28915554pfb.33.2016.06.06.09.32.09 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 06 Jun 2016 09:32:10 -0700 (PDT) From: "Qing" To: , Cc: , , , References: <201606021751.u52HpTrH090384@repo.freebsd.org> <3448221465067132@web17h.yandex.ru> <3505271465193212@web29h.yandex.ru> In-Reply-To: <3505271465193212@web29h.yandex.ru> Subject: RE: svn commit: r301217 - in head/sys: net netinet netinet6 Date: Mon, 6 Jun 2016 09:32:08 -0700 Message-ID: <000001d1c010$f8d603c0$ea820b40$@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 15.0 Thread-Index: AQHlai0SVLpjmg1ru461iLTBWDNtEwFUZxBSAak1TRwA5ZA64J+WMCqQ Content-Language: en-us X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jun 2016 16:32:11 -0000 -----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 Cc: Mike Karels ; src-committers = ; svn-src-all ; = svn-src-head Subject: Re: svn commit: r301217 - in head/sys: net netinet netinet6 06.06.2016, 04:40, "George Neville-Neil" : > On 4 Jun 2016, at 15:05, Alexander V. Chernikov wrote: > >> 02.06.2016, 20:51, "George V. Neville-Neil" : >>> 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