Date: Fri, 2 Dec 2011 20:02:46 -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: <201112022002.49618.jkim@FreeBSD.org> In-Reply-To: <201112021927.25234.jkim@FreeBSD.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <4ED8575D.2010405@freebsd.org> <201112021927.25234.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 02 December 2011 07:27 pm, Jung-uk Kim wrote: > 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_i > > >>nt 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 I booted up the kernel and found it just crashes because of stupid typos. Now a new patch is uploaded in place. Sorry. Anyway, I see no regression nor ABI breakage. :-) Jung-uk Kim > 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?201112022002.49618.jkim>