svn commit: r227778 - head/sys/net

[Lawrence Stewart]

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:
>>>
>>> Thanks all for the feedback. With some delay, I have a patch against
>>> r227871 that implements what Lawrence proposed. You can find it
>>> here:
>>> http://www.cubinlab.ee.unimelb.edu.au/~jrid/patches/ffclock-bpf-header-r227871.patch
>>>
>>
>> There are a few nits, but the patch implements what I envisaged,
>> thanks Julien.
>
> The use of the union for bh_ustamp feels a bit odd, as (e.g.) a bpf_ts
> is two 64-bit ints, but an ffcounter is just a single 64-bit int.

Julien and I discussed this - the alternative of somehow casting that 
struct member to something else is probably worse, so we're going to 
stick with a union.

>>
>>> I have tested this under a few typical scenario, it works as
>>> expected but already brings some headaches (hence the long delay
>>> mentioned above :-)).
>>>
>>> I thought a bit more of user cases. I believe many of them call for
>>> having both feed-forward counter and its conversion in second be
>>> present in the BPF header. For example, this allows to have absolute
>>> packet departure/arrival times (as per usual), but also provides the
>>> opportunity to compute inter-arrival times accurately using the
>>> difference clock. There are other examples I can think of, and if one
>>> believe the feed-forward clock approach becomes more popular, such
>>> usages will be more and more common.
>>>
>
> Having read only the first introductory link that Lawrence posted when
> he first started introducing the ffclock code, it does really seem like
> there are lots of interesting things to do with both timestamps available.
>
>
>>> Assuming the BPF header grows by 8 bytes independent of any kernel
>>> option, I admit that the current implementation is a bit ugly. The
>>> BPF structure is not nicely packed and looks clunky. Ideally, the
>
> The
> +#define bh_tstamp bh_ustamp.ts_stamp
> is a sort of thing that can get annoying when poking around kernel
> cores, &c. I won't argue with you that the current implementation is a
> bit ugly.

Agreed it's gross, but in sticking with the union route, this is a 
necessary evil to reduce the impact of integrating FFCLOCK support into BPF.

>>> feed-forward counter should be placed just below the bh_tstamp
>>> member, but this would require libpcap and all ports depending on it
>>> to be recompiled after this change.
>>
>> Even though it looks a bit gross, we would still add it at the end to
>> avoid gratuitously breaking binaries. We would then also add some
>> explicit padding in the struct to soak up the redundant space left in
>> between it and the second last struct member.
>
> Though ... we are just after a release branch is forked. That seems to
> be a much better time to change the ABI for cleanliness' sake than right
> before a release ;)

True.

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

Cheers,
Lawrence