From owner-svn-src-all@FreeBSD.ORG Sat Dec 3 04:30:58 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 E601F106566B; Sat, 3 Dec 2011 04:30:58 +0000 (UTC) (envelope-from jrid@cubinlab.ee.unimelb.edu.au) Received: from outbound.icp-qv1-irony-out1.iinet.net.au (outbound.icp-qv1-irony-out1.iinet.net.au [203.59.1.106]) by mx1.freebsd.org (Postfix) with ESMTP id 5A8188FC08; Sat, 3 Dec 2011 04:30:56 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApIBADCe2U58lVQR/2dsb2JhbAAMOBatDgEBAQEDOj8QCw05VwaIIq03iQWDbYZRYwSaP4xE X-IronPort-AV: E=Sophos;i="4.71,287,1320595200"; d="scan'208";a="822330695" Received: from unknown (HELO [192.168.0.16]) ([124.149.84.17]) by outbound.icp-qv1-irony-out1.iinet.net.au with ESMTP; 03 Dec 2011 12:02:48 +0800 Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Julien Ridoux In-Reply-To: <201112022002.49618.jkim@FreeBSD.org> Date: Sat, 3 Dec 2011 15:02:47 +1100 Content-Transfer-Encoding: quoted-printable Message-Id: <4D87C75E-5C49-4D6D-BFD7-4A1E1C2B1C13@cubinlab.ee.unimelb.edu.au> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <4ED8575D.2010405@freebsd.org> <201112021927.25234.jkim@FreeBSD.org> <201112022002.49618.jkim@FreeBSD.org> To: Jung-uk Kim X-Mailer: Apple Mail (2.1084) Cc: src-committers@FreeBSD.org, John Baldwin , svn-src-all@FreeBSD.org, Ben Kaduk , Benjamin Kaduk , svn-src-head@FreeBSD.org, Lawrence Stewart 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: Sat, 03 Dec 2011 04:30:59 -0000 On 03/12/2011, at 12:02 PM, Jung-uk Kim wrote: [snip] >>>>>=20 >>>>> - 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(). >>>>=20 >>>> I did that to reduce branching. Since you are introducing more >>>> branches, it warrants a function pointer now. >>=20 >> There's another reason, BTW. If mbufs are tagged with timestamps >> (where only monotonic timestamps are allowed), they must be >> converted within the bpf.c. I forgot all the details. :-( >>=20 >>> 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. >>>=20 >>> 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. >>>=20 >>> 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. >>=20 >> Please see my patch: >>=20 >> http://people.freebsd.org/~jkim/bpf_ffclock.diff >=20 > I booted up the kernel and found it just crashes because of stupid=20 > typos. Now a new patch is uploaded in place. Sorry. >=20 > Anyway, I see no regression nor ABI breakage. :-) >=20 > Jung-uk Kim >=20 >> This is what I originally intended to propose (just >> compile-tested). I did not use union *intentionally* to make the >> patch simpler. Instead, I added BPF_FFCOUNT() macro. It's kinda >> ugly but I don't see any compelling reason to introduce the union >> (yet). >>=20 >>> 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. >>=20 >> It does not affect user applications as bpfdesc.h is only meant for >> kernel uses. Also, it is very hard for me to imagine any >> third-party drivers that use struct bpf_d directly. >>=20 >> Jung-uk Kim Jung-uk, thanks for the patch, I understand much better. Overall I am definitely not against this patch. I see two minor issues = that require some attention though. Using the higher level ffclock_[get]binuptime() instead of = ffclock_abstime() adds an extra function call. My performance metric of = interest is timestamping latency, because all things being equal, the = variablity of the timestamping delay gives a less accurate picture of = the network and affects the synchronisation daemon and the final quality = of the clock. Although I agree that the patch we proposed with Lawrence can be = improved on this front, adding one more level of abstraction to the = timestamping function goes the opposite direction. The earlier the = timestamp is created, the better the results. I am definitely not very savvy on the code optimisation front, and I = need to be educated there. I am wondering if removing the branching in = the code is worth this extra variability in latency.=20 Looking ahead to the next patch we want to propose, the BPF header will = contain a timestamp in seconds from one of the two clocks and a = feed-forward counter. Here is a very simplistic proof of concept based = on Lawrence's patch. I just put it there to support the discussion: = http://www.cubinlab.ee.unimelb.edu.au/~jrid/patches/r228191/bpfffcounter_c= oncept_v2.patch Since it is essential to read the hardware counter only once to produce = the timestamp in second and the ffcounter, the prototype of the = timestamping function stored in the bpf_d would have to be changed (if = the kernel is compiled with the FFCLOCK option, at least). For example, if BPF_T_FFCLOCK is set, this change will make use of = ffclock_abstime() and solves the performance issue I raised above. If the clock flag is not set, then the [get]binuptime() will need a = wrapper function, and they will bear the timestamping latency cost. Is this acceptable? If yes, then I don't have any objections to the = patch. Julien