svn commit: r227778 - head/sys/net

[Lawrence Stewart]

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?

Cheers,
Lawrence