From owner-svn-src-all@FreeBSD.ORG Wed Nov 23 06:42:19 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 22742106564A; Wed, 23 Nov 2011 06:42:19 +0000 (UTC) (envelope-from jrid@cubinlab.ee.unimelb.edu.au) Received: from mail-gw1.its.unimelb.edu.au (mail-gw1.its.unimelb.edu.au [128.250.5.150]) by mx1.freebsd.org (Postfix) with ESMTP id C911B8FC16; Wed, 23 Nov 2011 06:42:18 +0000 (UTC) Received: from emu.cubinlab.ee.unimelb.edu.au (cubinlab.ee.unimelb.edu.au [128.250.80.33]) by mail-gw1.its.unimelb.edu.au (Postfix) with ESMTPS id 57A4CE50; Wed, 23 Nov 2011 17:42:17 +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 pAN6gGBh098328 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES128-SHA bits=128 verify=NO); Wed, 23 Nov 2011 17:42:17 +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: <4ECBAB19.4010907@freebsd.org> Date: Wed, 23 Nov 2011 17:42:16 +1100 Content-Transfer-Encoding: quoted-printable Message-Id: <8805563E-60F9-4BF4-A292-1A43C4FA368A@cubinlab.ee.unimelb.edu.au> 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> 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 , John Baldwin 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: Wed, 23 Nov 2011 06:42:19 -0000 On 23/11/2011, at 1:00 AM, Lawrence Stewart wrote: > On 11/23/11 00:30, John Baldwin wrote: >> On Monday, November 21, 2011 2:28:10 am 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: >>>>>> On Sun, Nov 20, 2011 at 11:17 PM, Lawrence >>>>>> Stewart wrote: >>>>>>> Author: lstewart Date: Mon Nov 21 04:17:24 2011 New Revision: >>>>>>> 227778 URL: http://svn.freebsd.org/changeset/base/227778 >>>>>>>=20 >>>>>>> Log: - When feed-forward clock support is compiled in, change >>>>>>> the BPF header to contain both a regular timestamp obtained >>>>>>> from the system clock and the current feed-forward ffcounter >>>>>>> value. This enables new possibilities including >>>>>>=20 >>>>>> 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 >>> As to the issue of how a kernel module would detect if it's being = loaded >>> into a FFCLOCK enabled kernel, why wouldn't we expect modules to >>> "#include opt_ffclock.h" and conditionally compile code based on = FFCLOCK >>> being defined? Is there a use case for run-time (as opposed to >>> compile-time) module detection of feed-forward clock capabilities? >>=20 >> Think of standalone modules that are not built as part of a kernel = (e.g. >> 3rd party device drivers). In general we should avoid having = structures >> change size for kernel options, especially common structures. It = just adds >> lots of pain and suffering and complexity. We are stuck with it for = PAE on >> i386 (which causes pain), and for LOCK_PROFILING (but that is = sufficiently >> rare and expensive it seems to be ok). I think 8 bytes for bpf = packet is >> not sufficiently expensive to justify the extra headache. Just = always leave >> the new field in. >=20 > hmm... Julien almost has a patch finished which accomplishes what my = most recent email in this thread describes. Julien, I suggest we get it = finished and follow up to this thread with a pointer to the patch for = people to look at. If there's still a strong feeling that what it does = is going to bring pain we can do away with the new BPF_FFCOUNTER config = option and have the bpf header struct just grow by 8 bytes. >=20 > Stay tuned... 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-r22= 7871.patch 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. 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 = 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. What is your favourite option? Julien=