Date: Mon, 21 Nov 2011 19:28:35 +1100 From: Lawrence Stewart <lstewart@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: <4ECA0BB3.4000208@freebsd.org> In-Reply-To: <4EC9FD8A.5040401@freebsd.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <CAK2BMK4DP=japDufnbMUgqMgmL7rRye4wMrwqzHePyreNwiu-Q@mail.gmail.com> <4EC9E408.9000304@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 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4ECA0BB3.4000208>