Date: Wed, 30 Nov 2011 00:05:33 +1100 From: Lawrence Stewart <lstewart@freebsd.org> To: Benjamin Kaduk <kaduk@mit.edu> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au>, Ben Kaduk <minimarmot@gmail.com>, svn-src-head@freebsd.org Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <4ED4D89D.9000904@freebsd.org> In-Reply-To: <alpine.GSO.1.10.1111272248000.882@multics.mit.edu> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <648D11A8-3636-49E5-BF20-83E4EA87242C@cubinlab.ee.unimelb.edu.au> <4EC9FD8A.5040401@freebsd.org> <201111220830.05029.jhb@freebsd.org> <4ECBAB19.4010907@freebsd.org> <8805563E-60F9-4BF4-A292-1A43C4FA368A@cubinlab.ee.unimelb.edu.au> <4ECCA2A1.4020408@freebsd.org> <alpine.GSO.1.10.1111272248000.882@multics.mit.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4ED4D89D.9000904>