On 11/21/11 18:28, 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:
[snip]
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.
I think I might be changing my mind about the benefits of having both
the timestamp and the ffcounter value in the header. Adding an extra 8
bytes to the bpf header size does give us the ability to do some neat
things in a dual system clock world, but on further reflection these
things may not be worth the size increase cost for the average FreeBSD user.
On further reflection, I suspect the number of people likely to be
interested in having the flexibility of retaining both the timestamp and
ffcounter value would be less than those who don't care. We could
therefore expect that anyone wanting the flexibility should patch their
kernel and rebuild kernel/world in order to handle the ABI breakage and
put things in sync.
Given that ffcounter is currently a 64bit value, we could add a
CTASSERT() safety belt somewhere to check that sizeof(ffcounter) ==
sizeof(struct timeval) and then retask the net.bpf.ffclock_tstamp sysctl
(or perhaps better rename it to something like "net.bpf.timestamp_src")
and have it take options like "fb_time", "ff_time" or "ff_counter" for
feedback clock based timestamp, feed-forward based timestamp, or
feed-forward counter based timestamp.
We would then have the bpf code shoehorn either a regular timeval
timestamp or the ffcounter value into bh_tstamp header field depending
on whether "fb_time"/"ff_time" or "ff_counter" is selected. Anything
consuming the BPF header would then need to know how to interpret the
bh_tstamp field. Perhaps we could add a general purpose flags field to
the BPF header and define a flag which indicates if the bh_tstamp field
is a timestamp or counter value.
Thoughts?
Cheers,
Lawrence
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
