From owner-svn-src-all@freebsd.org Mon Jun 6 16:58:23 2016 Return-Path: Delivered-To: svn-src-all@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 6DE0EB6D2B4; Mon, 6 Jun 2016 16:58:23 +0000 (UTC) (envelope-from tomelite82@gmail.com) Received: from mail-oi0-x232.google.com (mail-oi0-x232.google.com [IPv6:2607:f8b0:4003:c06::232]) (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 37F7A1FF1; Mon, 6 Jun 2016 16:58:23 +0000 (UTC) (envelope-from tomelite82@gmail.com) Received: by mail-oi0-x232.google.com with SMTP id p204so40113826oih.3; Mon, 06 Jun 2016 09:58:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=R0Hxekaxp3MCMg93BwRpV8cex7I+wnXrUBsY+wliC/Y=; b=E2gl/k4AL4ktuULTvV/9P10+Yt9AoTUhR1nAvkZQX15rWLYc/TvgWLbySVQVOn8ZqB 5emTz62T0n5W1WBE2yz8hmpkIke5cVJkTEz0fO7RgK/zaIPFYTQ7JY75fHY64k1pVhH6 CYjtJyqxkbutT4dZAdfvrwZAE8TO1xOG1figYgzv/LP4WDZkfMTAb1lPRuIbjwybs9gm v3TmtboKEMfT7PvHVVqkjlDtDqThtZpddT+zfzMxm/bq8rjEz4IQKudTcZl3iiYP7Nsn LjKTo89a2lCcO0mBxOLvrQZffP+ZClM0y+HPSGFDyfl8Ev5OuiFnq9LFOo4ADsME9vAd 4NIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=R0Hxekaxp3MCMg93BwRpV8cex7I+wnXrUBsY+wliC/Y=; b=j60BJvHMzioVXDRAqpTQuNJNglu2pZf6RMY9TLkgvNlUgap2n4S1eTsFd8Y76HCygd m9iems+zmsrwPqDLPPiwnrjQI0iDz62T8hqnSr1IzTDCh2T0Q1wjzNnPGp0UfBcdEsJP g0xpRJUkWnWgYkWn9TURPV8TbnQPhRBVoSZhVHW7R4eux8ezx3cfwNbMdPKyNli5I0e7 BcR/xdlLjOc/x2iuF1qG5WozpbRLKPw1X6b9nYxxQNlQDIHk0Tdn2Dbj+jWhyScxUbAm /6uRx5UVsCNd2gTr2oyf8CZbjxeS7/0Njq7EbMXmHMUCmQ9HFFv51I10/BNRLyzb5NjD TsKQ== X-Gm-Message-State: ALyK8tLdSmjBXuZgUmoEAFui2krcs3gSpMsFhYi2YuBON2j34W+pZaRHPL7Lq+64VlgIE08m4jpaEubPGKVoXg== X-Received: by 10.157.56.38 with SMTP id i35mr5589032otc.26.1465232302383; Mon, 06 Jun 2016 09:58:22 -0700 (PDT) MIME-Version: 1.0 Sender: tomelite82@gmail.com Received: by 10.182.86.35 with HTTP; Mon, 6 Jun 2016 09:58:21 -0700 (PDT) In-Reply-To: <3505271465193212@web29h.yandex.ru> References: <201606021751.u52HpTrH090384@repo.freebsd.org> <3448221465067132@web17h.yandex.ru> <3505271465193212@web29h.yandex.ru> From: Qing Li Date: Mon, 6 Jun 2016 09:58:21 -0700 X-Google-Sender-Auth: rbPTP-dd0H9_WLb8Z0snqsh43oA Message-ID: Subject: Re: svn commit: r301217 - in head/sys: net netinet netinet6 To: "Alexander V. Chernikov" Cc: George Neville-Neil , Mike Karels , src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jun 2016 16:58:23 -0000 On Sun, Jun 5, 2016 at 11:06 PM, Alexander V. Chernikov < melifaro@freebsd.org> wrote: > 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 cou= ld 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 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 =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 thi= s 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=9Cpl= le=E2=80=9D > >> is NULL. > >> > >> This can be easily verified by calling something like > >> dtrace -n 'fbt:kernel:ether_output:entry /arg3!=3DNULL&&((struct rout= e > >> *)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 > > 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