svn commit: r227778 - head/sys/net

Sat Dec 3 01:03:03 UTC 2011

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