svn commit: r227778 - head/sys/net
jhb at freebsd.org
Tue Nov 22 13:30:08 UTC 2011
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 at 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.
More information about the svn-src-head