From owner-svn-src-all@FreeBSD.ORG Mon Nov 21 09:52:10 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CEA6E1065672; Mon, 21 Nov 2011 09:52:10 +0000 (UTC) (envelope-from jrid@cubinlab.ee.unimelb.edu.au) Received: from mail-gw2.its.unimelb.edu.au (mail-gw2.its.unimelb.edu.au [128.250.5.151]) by mx1.freebsd.org (Postfix) with ESMTP id 860828FC0C; Mon, 21 Nov 2011 09:52:10 +0000 (UTC) Received: from emu.cubinlab.ee.unimelb.edu.au (cubinlab.ee.unimelb.edu.au [128.250.80.33]) by mail-gw2.its.unimelb.edu.au (Postfix) with ESMTPS id 40987EE2; Mon, 21 Nov 2011 20:52:09 +1100 (EST) Received: from jrid.cubinlab.ee.unimelb.edu.au (jrid.cubinlab.ee.unimelb.edu.au [10.0.1.128]) (authenticated bits=0) by emu.cubinlab.ee.unimelb.edu.au (8.14.4/8.14.4) with ESMTP id pAL9q7Q2055302 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES128-SHA bits=128 verify=NO); Mon, 21 Nov 2011 20:52:08 +1100 (EST) (envelope-from jrid@cubinlab.ee.unimelb.edu.au) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Julien Ridoux In-Reply-To: <4ECA0BB3.4000208@freebsd.org> Date: Mon, 21 Nov 2011 20:52:07 +1100 Content-Transfer-Encoding: quoted-printable Message-Id: <62D53750-FB6E-4DF3-A1E2-65B22EF85EB6@cubinlab.ee.unimelb.edu.au> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <4EC9E408.9000304@freebsd.org> <648D11A8-3636-49E5-BF20-83E4EA87242C@cubinlab.ee.unimelb.edu.au> <4EC9FD8A.5040401@freebsd.org> <4ECA0BB3.4000208@freebsd.org> To: Lawrence Stewart X-Mailer: Apple Mail (2.1084) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Ben Kaduk Subject: Re: svn commit: r227778 - head/sys/net X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Nov 2011 09:52:10 -0000 On 21/11/2011, at 7:28 PM, Lawrence Stewart wrote: > On 11/21/11 18:28, Lawrence Stewart wrote: >> On 11/21/11 17:18, Julien Ridoux wrote: >>>=20 >>> On 21/11/2011, at 4:39 PM, Lawrence Stewart wrote: >>>=20 >>>> On 11/21/11 16:12, Ben Kaduk wrote: > [snip] >>>>> Is it really necessary to make the ABI dependent on a kernel >>>>> configuration option? This causes all sorts of headaches if >>>>> loadable modules ever want to use that ABI, something that we >>>>> just ran into with vm_page_t and friends and had a long thread on >>>>> -current about. >>>>=20 >>>> Fair question. Julien, if pcap and other consumers will happily >>>> ignore the new ffcount_stamp member in the bpf header, is there any >>>> reason to conditionally add the ffcounter into the header struct? >>>=20 >>> It is a valid question indeed. The feedback I have received so far >>> was to not have the feed-forward clock support be a default kernel >>> configuration option. What follows is based on this assumption. >>>=20 >>> The commit (r227747) introduces sysctl that are conditioned by the >>> same "FFCLOCK" kernel configuration option. If a loadable module >>> tests for the presence of this sysctl, it will know if the >>> ffcount_stamp member is available. Is it too much of a hack? >>>=20 >>> Alternatively, if the ffcounter is added to the bpf header >>> unconditionally, the ffcount_stamp member can be set to 0. Loadable >>> modules will then see a consistent ABI but will retrieve a >>> meaningless value. >>>=20 >>> I am not sure which option makes more sense, any preference? >>=20 >> If I understand the issues correctly, I think the appropriate path >> forward is to remove the conditional change to the bpf header and = have >> ffcount_stamp become a permanent member of the struct. We'll just = leave >> the member uninitialised in the !FFCLOCK case. This change will make = the >> patch un-MFCable, but I think that's ok. >=20 > I think I might be changing my mind about the benefits of having both = the timestamp and the ffcounter value in the header. Adding an extra 8 = bytes to the bpf header size does give us the ability to do some neat = things in a dual system clock world, but on further reflection these = things may not be worth the size increase cost for the average FreeBSD = user. >=20 > On further reflection, I suspect the number of people likely to be = interested in having the flexibility of retaining both the timestamp and = ffcounter value would be less than those who don't care. We could = therefore expect that anyone wanting the flexibility should patch their = kernel and rebuild kernel/world in order to handle the ABI breakage and = put things in sync. >=20 > Given that ffcounter is currently a 64bit value, we could add a = CTASSERT() safety belt somewhere to check that sizeof(ffcounter) =3D=3D = sizeof(struct timeval) and then retask the net.bpf.ffclock_tstamp sysctl = (or perhaps better rename it to something like "net.bpf.timestamp_src") = and have it take options like "fb_time", "ff_time" or "ff_counter" for = feedback clock based timestamp, feed-forward based timestamp, or = feed-forward counter based timestamp. >=20 > We would then have the bpf code shoehorn either a regular timeval = timestamp or the ffcounter value into bh_tstamp header field depending = on whether "fb_time"/"ff_time" or "ff_counter" is selected. Anything = consuming the BPF header would then need to know how to interpret the = bh_tstamp field. Perhaps we could add a general purpose flags field to = the BPF header and define a flag which indicates if the bh_tstamp field = is a timestamp or counter value. >=20 > Thoughts? In a typical scenario with a feed-forward clock, the synchronisation = daemon needs the ffcounter timestamps. Tcpdump (or any libpcap based = processed) would expects a timetamp in seconds formatted as a timeval. If the ffcounter value is stored as an extra value in the bpf header, = both processes are happy. If the current timestamp field is used, then the system wide sysctl will = not suffice. This will require to modify/add bpf ioctl to be able to = configure each open bpf independently. Also, selecting a timestamp in seconds would prevent packet traces from = being replayed and have their timestamps corrected in post-processing. = However, I agree that this feature and the capacity to compare 2 = different synchronisation algorithms running in parallel is attractive = to a small proportion of FreeBSD users.