Skip site navigation (1)Skip section navigation (2)
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>