Date: Sat, 3 Dec 2011 15:02:47 +1100 From: Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au> To: Jung-uk Kim <jkim@freebsd.org> Cc: src-committers@FreeBSD.org, John Baldwin <jhb@FreeBSD.org>, svn-src-all@FreeBSD.org, Ben Kaduk <minimarmot@gmail.com>, Benjamin Kaduk <kaduk@mit.edu>, svn-src-head@FreeBSD.org, Lawrence Stewart <lstewart@FreeBSD.org> Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <4D87C75E-5C49-4D6D-BFD7-4A1E1C2B1C13@cubinlab.ee.unimelb.edu.au> In-Reply-To: <201112022002.49618.jkim@FreeBSD.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <4ED8575D.2010405@freebsd.org> <201112021927.25234.jkim@FreeBSD.org> <201112022002.49618.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4D87C75E-5C49-4D6D-BFD7-4A1E1C2B1C13>