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.
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.
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 ;)
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.
Thanks,
-Ben Kaduk
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
