Date: Fri, 2 Dec 2011 19:27:04 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: Lawrence Stewart <lstewart@freebsd.org> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au>, Ben Kaduk <minimarmot@gmail.com>, Benjamin Kaduk <kaduk@mit.edu>, svn-src-head@freebsd.org Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <201112021927.25234.jkim@FreeBSD.org> In-Reply-To: <4ED8575D.2010405@freebsd.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <201112011143.27707.jkim@FreeBSD.org> <4ED8575D.2010405@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 01 December 2011 11:43 pm, Lawrence Stewart wrote: > 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_i > >>>>nt 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_int > >>act 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. 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. :-( > 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. Please see my patch: http://people.freebsd.org/~jkim/bpf_ffclock.diff 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). > 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. 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. Jung-uk Kim
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112021927.25234.jkim>