Date: Tue, 22 Nov 2011 08:30:04 -0500 From: John Baldwin <jhb@freebsd.org> To: Lawrence Stewart <lstewart@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au>, Ben Kaduk <minimarmot@gmail.com> Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <201111220830.05029.jhb@freebsd.org> In-Reply-To: <4EC9FD8A.5040401@freebsd.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <648D11A8-3636-49E5-BF20-83E4EA87242C@cubinlab.ee.unimelb.edu.au> <4EC9FD8A.5040401@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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<lstewart@freebsd.org> 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. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201111220830.05029.jhb>