From owner-svn-src-head@FreeBSD.ORG Tue Nov 29 13:05:36 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7DAA4106566B; Tue, 29 Nov 2011 13:05:36 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id E3EEB8FC18; Tue, 29 Nov 2011 13:05:35 +0000 (UTC) Received: from lstewart1.loshell.room52.net (ppp59-167-184-191.static.internode.on.net [59.167.184.191]) by lauren.room52.net (Postfix) with ESMTPSA id 0E2EB7E820; Wed, 30 Nov 2011 00:05:33 +1100 (EST) Message-ID: <4ED4D89D.9000904@freebsd.org> Date: Wed, 30 Nov 2011 00:05:33 +1100 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:7.0.1) Gecko/20111016 Thunderbird/7.0.1 MIME-Version: 1.0 To: Benjamin Kaduk 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> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=unavailable version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on lauren.room52.net Cc: src-committers@freebsd.org, John Baldwin , svn-src-all@freebsd.org, Julien Ridoux , Ben Kaduk , svn-src-head@freebsd.org Subject: Re: svn commit: r227778 - head/sys/net X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Nov 2011 13:05:36 -0000 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