Date: Sun, 27 Nov 2011 22:59:01 -0500 (EST) From: Benjamin Kaduk <kaduk@MIT.EDU> To: Lawrence Stewart <lstewart@freebsd.org> 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: <alpine.GSO.1.10.1111272248000.882@multics.mit.edu> In-Reply-To: <4ECCA2A1.4020408@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.GSO.1.10.1111272248000.882>