Re: Improving the data supplied by BPF

["Arnaud Lacombe" <lacombar%gmail.com@localhost>]

On Thu, Dec 25, 2008 at 6:34 PM, Christos Zoulas 
<christos%astron.com@localhost> wrote:
> In article 
> <1a69a9d80812251258s425cd795g40bf787f74e82566%mail.gmail.com@localhost>,
> Arnaud Lacombe <lacombar%gmail.com@localhost> wrote:
>
>>> The purpose of the sequence number is to provide the rolling counter
>>> of the packets captured for the one in question. Thus if in successive
>>> reads the count went from 2 to 5, you know 3 packets have been missed.
>>>
>>what if the count goes from 3 to... 3, ie. the seq number overflowed
>>(for whatever reason) ?
>
> Highly unlinkely.
>
2^32 1500-bytes packets is about 6TB of data, on 100Mbit link, the
sequence number will wrap after 5.6 days (if you consider
uni-directional traffic), on a 1Gb link, half a day and a bit more
than 1 hour on a 10Gb link. This is the worst case scenario. Two
records taken at the <wrap_time> interval will likely collide on
high-load link.

>>why unsigned ? currently `tv_sec' is signed. Why not using time_t ?
>>There is an obvious ABI breakage when we will switch to 64bits time_t
>>but this is be a better type than raw integer. The breakage is a
>>different trouble and should be dealt with separately.
>
> You answered your own question. All types in the struct should be fixed size
> and time_t will change soon to be 64 bits.
>
This is not the problem here.

The ABI breakage will anyway cause a problem with all system call
taking a `struct timespec' as a parameter or including a `struct
timespec' in one of their parameter.

>>>       uint32_t        ebr_nsecs;
>>>
>>why do you want nano second precision if you getting your information
>>from a micro second precision variable. There is no information gain
>>there, and your code reflect this (ie. you just "* 1000" to get the
>>nano second value from the micro second value).
>
> In the time_t branch it already uses nanoseconds. Plus why you would want to
> stick with micros in the new interface?
>
The patch I commented used a micro value to get a nano value. I don't
see your point. I don't get the link between Darren's patch and the
time_t branch either.

>>This field would have a meaning if you change the call the call to
>>microtime() to nanotime() in bpf_tap()/bpd_deliver() and build a
>>homegrown `struct timeval' in the non-extended capture format. You
>>don't have any precision loss in that case.
>>
>>btw, why not just using a `struct timespec' ?
>
> fixed sizes. Most on wire structures use fixed sizes. this allows portability
> across different architectures.
>
the current BPF don't... It use a `struct timeval' whose tv_sec and
tv_usec are `long' which is not portable across architecture (i386's
long == 4, amd64's long == 8).

>>>       uint32_t        ebr_seqno;      /* sequence number in capture */
>>how to detect wrap in sequence number ?
>
> It does not happen in real situations.
>
I'm not no so sure about this, cf above.

btw, why is the current `struct bpf_hdr' not packed ? this would avoid
the SIZEOF_BPF_HDR hack...

 - Arnaud