Date: Mon, 21 Nov 2011 20:52:07 +1100 From: Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au> To: Lawrence Stewart <lstewart@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Ben Kaduk <minimarmot@gmail.com> Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <62D53750-FB6E-4DF3-A1E2-65B22EF85EB6@cubinlab.ee.unimelb.edu.au> In-Reply-To: <4ECA0BB3.4000208@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> <4ECA0BB3.4000208@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21/11/2011, at 7:28 PM, Lawrence Stewart wrote: > On 11/21/11 18:28, Lawrence Stewart wrote: >> On 11/21/11 17:18, Julien Ridoux wrote: >>>=20 >>> On 21/11/2011, at 4:39 PM, Lawrence Stewart wrote: >>>=20 >>>> 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. >>>>=20 >>>> 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? >>>=20 >>> 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. >>>=20 >>> 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? >>>=20 >>> 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. >>>=20 >>> I am not sure which option makes more sense, any preference? >>=20 >> 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. >=20 > 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. >=20 > 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. >=20 > Given that ffcounter is currently a 64bit value, we could add a = CTASSERT() safety belt somewhere to check that sizeof(ffcounter) =3D=3D = 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. >=20 > 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. >=20 > Thoughts? In a typical scenario with a feed-forward clock, the synchronisation = daemon needs the ffcounter timestamps. Tcpdump (or any libpcap based = processed) would expects a timetamp in seconds formatted as a timeval. If the ffcounter value is stored as an extra value in the bpf header, = both processes are happy. If the current timestamp field is used, then the system wide sysctl will = not suffice. This will require to modify/add bpf ioctl to be able to = configure each open bpf independently. Also, selecting a timestamp in seconds would prevent packet traces from = being replayed and have their timestamps corrected in post-processing. = However, I agree that this feature and the capacity to compare 2 = different synchronisation algorithms running in parallel is attractive = to a small proportion of FreeBSD users.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?62D53750-FB6E-4DF3-A1E2-65B22EF85EB6>