svn commit: r227778 - head/sys/net

[Lawrence Stewart]

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_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.

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. 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.

Other ideas would be very welcome.

Cheers,
Lawrence