From owner-svn-src-head@FreeBSD.ORG Tue Nov 22 14:01:00 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 0E5341065670; Tue, 22 Nov 2011 14:01:00 +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 9A5098FC12; Tue, 22 Nov 2011 14:00:59 +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 69E917E880; Wed, 23 Nov 2011 01:00:57 +1100 (EST) Message-ID: <4ECBAB19.4010907@freebsd.org> Date: Wed, 23 Nov 2011 01:00:57 +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: John Baldwin 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> In-Reply-To: <201111220830.05029.jhb@freebsd.org> 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: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Julien Ridoux , Ben Kaduk 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, 22 Nov 2011 14:01:00 -0000 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: >>> >>> On 21/11/2011, at 4:39 PM, Lawrence Stewart wrote: >>> >>>> 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 >>>>>> >>>>>> 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 >>>>> >>>>> 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. >>>> >>>> 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? >>> >>> 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. >>> >>> 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? >>> >>> 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. >>> >>> I am not sure which option makes more sense, any preference? >> >> 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. >> >> 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? > > 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. 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. Stay tuned... Cheers, Lawrence