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
_______________________________________________
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"
