Date: Thu, 1 Dec 2011 11:43:25 -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: <201112011143.27707.jkim@FreeBSD.org> In-Reply-To: <4ED79930.60307@freebsd.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <201111291309.20419.jkim@FreeBSD.org> <4ED79930.60307@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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_int > >>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_intact >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. Jung-uk Kim > - bpf.4 man page updated > > - Proposed commit message at top of patch updated > > > If you'd like to look it over closely but can't do it in the next > 10 hours or so, let me know and I'll postpone committing. > > Cheers, > Lawrence
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112011143.27707.jkim>