From owner-svn-src-all@FreeBSD.ORG Fri Dec 2 04:43:12 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 56864106566C; Fri, 2 Dec 2011 04:43:12 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id BCB608FC19; Fri, 2 Dec 2011 04:43:11 +0000 (UTC) Received: from lstewart.caia.swin.edu.au (lstewart.caia.swin.edu.au [136.186.229.95]) by lauren.room52.net (Postfix) with ESMTPSA id 3E49E7E820; Fri, 2 Dec 2011 15:43:10 +1100 (EST) Message-ID: <4ED8575D.2010405@freebsd.org> Date: Fri, 02 Dec 2011 15:43:09 +1100 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:8.0) Gecko/20111127 Thunderbird/8.0 MIME-Version: 1.0 To: Jung-uk Kim References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <201111291309.20419.jkim@FreeBSD.org> <4ED79930.60307@freebsd.org> <201112011143.27707.jkim@FreeBSD.org> In-Reply-To: <201112011143.27707.jkim@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=unavailable version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on lauren.room52.net Cc: src-committers@FreeBSD.org, John Baldwin , svn-src-all@FreeBSD.org, Julien Ridoux , Ben Kaduk , Benjamin Kaduk , svn-src-head@FreeBSD.org Subject: Re: svn commit: r227778 - head/sys/net X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 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: Fri, 02 Dec 2011 04:43:12 -0000 On 12/02/11 03:43, Jung-uk Kim wrote: > On Thursday 01 December 2011 10:11 am, Lawrence Stewart wrote: >> On 11/30/11 05:09, Jung-uk Kim wrote: >>> On Tuesday 29 November 2011 11:13 am, Lawrence Stewart wrote: >>>> On 11/30/11 00:05, Lawrence Stewart wrote: >>>>> On 11/28/11 14:59, Benjamin Kaduk wrote: >>>>>> On Wed, 23 Nov 2011, Lawrence Stewart wrote: >>>>>>> On 11/23/11 17:42, Julien Ridoux wrote: >>>> >>>> [snip] >>>> >>>>>>>> What is your favourite option? >>>>>>> >>>>>>> FreeBSD parlance is to ask what colour you would like to >>>>>>> paint the bikeshed ;) >>>>>>> >>>>>>> As I've never experienced the pain John refers to, I'll defer >>>>>>> to the wisdom of others on whether the proposed patch will >>>>>>> create pain down the road. I think it's ok, but if consensus >>>>>>> is 8bytes per packet isn't going to break the bank, I guess >>>>>>> we just go for it - but I guess I am cautious about this >>>>>>> route as we can push a lot of packets per second through the >>>>>>> stack. >>>>>> >>>>>> Since other people seem to be keeping quiet, I'll add that I'm >>>>>> in favor of just always adding the 8 bytes per packet. >>>>> >>>>> Julien and I discussed this at length today, and agree that for >>>>> head, we'll add the new bh_ffcounter member to the BPF header >>>>> unconditionally. >>>>> >>>>> Thanks to you and John for the input. >>>>> >>>>> I'm going to revert r227778 in order to start form a clean >>>>> slate, and add two separate patches. One will reintegrate >>>>> FFCLOCK support with BPF without breaking the ABI. A follow up >>>>> patch will bump the ffclock version and add the bh_ffcounter to >>>>> the bpf header (after the timestamp member). Then a final patch >>>>> will bump __FreeBSD_version and add a note to UPDATING about >>>>> recompiling to get kernel/world in sync, which should seal the >>>>> deal. >>>> >>>> Here's the first of the patches: >>>> >>>> http://people.freebsd.org/~lstewart/patches/misc/ffclock_bpf_int >>>> act abi_10.x.r228130.patch >>> >>> I only glanced at it but it looks very close to what I wanted to >>> suggest. >> >> Final candidate patch is at: >> >> http://people.freebsd.org/~lstewart/patches/misc/ffclock_bpf_intact >> abi_10.x.r228180.patch >> >> Assuming it passes the "make tinderbox" build I'm currently running >> and no further input is received from interested parties, I plan to >> commit it in ~10 hours. >> >> Changes since the r228130 patch I sent previously: >> >> - The new flags in bpf.h are added unconditionally so that they can >> always be referenced at compile time and a decision made at runtime >> as to whether a flag will be set or not. Using one of the new flags >> when the kernel doesn't have FFCLOCK compiled in results in the >> flag being ignored. An app should check for the existence of the >> "ffclock" kernel feature or the "kern.sysclock" sysctl tree before >> attempting to use the flags. >> >> - This patch will hopefully be MFCed at some point, so I added a >> CTASSERT to bpf.c to ensure that the ABI of structs bpf_hdr32, >> bpf_hdr and bpf_xhdr remains intact when FFCLOCK is enabled and the >> union of a ffcounter and struct timeval32/timeval/bpf_ts is >> switched in. >> >> - bpf_gettime() more comprehensively covers all the possible cases >> of flag combination and does sensible things for each case (even >> though some cases are rather silly). >> >> - The snippet of code at the beginning of catchpacket() that was >> manipulating the struct bintime derived from bpf_gettime() was >> gross and has been removed in favour of selecting the right >> {get}bin{up}time() function call in bpf_gettime(). > > I did that to reduce branching. Since you are introducing more > branches, it warrants a function pointer now. I see, thanks for the explanation. Could you elaborate a bit more about how you would implement the function pointer idea? I'm also curious in the !FFCLOCK case just how much overhead having the 2-layer nested if/else adds. I'm not an very optimisation savvy person, but I'm wondering if it's actually worth micro-optimising this code. My initial thoughts about your function pointer idea lead to adding a function pointer in the bpf_d and setting it to the appropriate function to get the timestamp from at bpf_d creation or ioctl time. Whilst I like this idea, I can't see how it would work given that the various functions involved in time/ffcounter stamp generation all have different signatures. We could have multiple variants of bpf_gettime() which each call the appropriate underlying functions to generate the appropriate stamp. Would add quite a lot of code but would reduce the overhead of calling bpf_gettime() to an indirect function call + the underlying stamp generation function call. This also solves the problem of multiple function signatures. We would also need to add the function pointer to the bpf_d struct which I guess breaks the ABI, something we're trying to avoid with this patch as we want to MFC it. Other ideas would be very welcome. Cheers, Lawrence