Date: Sat, 3 Dec 2011 15:02:47 +1100 From: Julien Ridoux <jrid@cubinlab.ee.unimelb.edu.au> To: Jung-uk Kim <jkim@freebsd.org> Cc: src-committers@FreeBSD.org, John Baldwin <jhb@FreeBSD.org>, svn-src-all@FreeBSD.org, Ben Kaduk <minimarmot@gmail.com>, Benjamin Kaduk <kaduk@mit.edu>, svn-src-head@FreeBSD.org, Lawrence Stewart <lstewart@FreeBSD.org> Subject: Re: svn commit: r227778 - head/sys/net Message-ID: <4D87C75E-5C49-4D6D-BFD7-4A1E1C2B1C13@cubinlab.ee.unimelb.edu.au> In-Reply-To: <201112022002.49618.jkim@FreeBSD.org> References: <201111210417.pAL4HOdi023556@svn.freebsd.org> <4ED8575D.2010405@freebsd.org> <201112021927.25234.jkim@FreeBSD.org> <201112022002.49618.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03/12/2011, at 12:02 PM, Jung-uk Kim wrote:
[snip]
>>>>>=20
>>>>> - The snippet of code at the beginning of catchpacket() that
>>>>> was manipulating the struct bintime derived from bpf_gettime()
>>>>> was gross and has been removed in favour of selecting the
>>>>> right {get}bin{up}time() function call in bpf_gettime().
>>>>=20
>>>> I did that to reduce branching. Since you are introducing more
>>>> branches, it warrants a function pointer now.
>>=20
>> There's another reason, BTW. If mbufs are tagged with timestamps
>> (where only monotonic timestamps are allowed), they must be
>> converted within the bpf.c. I forgot all the details. :-(
>>=20
>>> I see, thanks for the explanation. Could you elaborate a bit more
>>> about how you would implement the function pointer idea? I'm also
>>> curious in the !FFCLOCK case just how much overhead having the
>>> 2-layer nested if/else adds. I'm not an very optimisation savvy
>>> person, but I'm wondering if it's actually worth micro-optimising
>>> this code.
>>>=20
>>> My initial thoughts about your function pointer idea lead to
>>> adding a function pointer in the bpf_d and setting it to the
>>> appropriate function to get the timestamp from at bpf_d creation
>>> or ioctl time. Whilst I like this idea, I can't see how it would
>>> work given that the various functions involved in time/ffcounter
>>> stamp generation all have different signatures.
>>>=20
>>> We could have multiple variants of bpf_gettime() which each call
>>> the appropriate underlying functions to generate the appropriate
>>> stamp. Would add quite a lot of code but would reduce the
>>> overhead of calling bpf_gettime() to an indirect function call +
>>> the underlying stamp generation function call. This also solves
>>> the problem of multiple function signatures.
>>=20
>> Please see my patch:
>>=20
>> http://people.freebsd.org/~jkim/bpf_ffclock.diff
>=20
> I booted up the kernel and found it just crashes because of stupid=20
> typos. Now a new patch is uploaded in place. Sorry.
>=20
> Anyway, I see no regression nor ABI breakage. :-)
>=20
> Jung-uk Kim
>=20
>> This is what I originally intended to propose (just
>> compile-tested). I did not use union *intentionally* to make the
>> patch simpler. Instead, I added BPF_FFCOUNT() macro. It's kinda
>> ugly but I don't see any compelling reason to introduce the union
>> (yet).
>>=20
>>> We would also need to add the function pointer to the bpf_d
>>> struct which I guess breaks the ABI, something we're trying to
>>> avoid with this patch as we want to MFC it.
>>=20
>> It does not affect user applications as bpfdesc.h is only meant for
>> kernel uses. Also, it is very hard for me to imagine any
>> third-party drivers that use struct bpf_d directly.
>>=20
>> Jung-uk Kim
Jung-uk, thanks for the patch, I understand much better.
Overall I am definitely not against this patch. I see two minor issues =
that require some attention though.
Using the higher level ffclock_[get]binuptime() instead of =
ffclock_abstime() adds an extra function call. My performance metric of =
interest is timestamping latency, because all things being equal, the =
variablity of the timestamping delay gives a less accurate picture of =
the network and affects the synchronisation daemon and the final quality =
of the clock.
Although I agree that the patch we proposed with Lawrence can be =
improved on this front, adding one more level of abstraction to the =
timestamping function goes the opposite direction. The earlier the =
timestamp is created, the better the results.
I am definitely not very savvy on the code optimisation front, and I =
need to be educated there. I am wondering if removing the branching in =
the code is worth this extra variability in latency.=20
Looking ahead to the next patch we want to propose, the BPF header will =
contain a timestamp in seconds from one of the two clocks and a =
feed-forward counter. Here is a very simplistic proof of concept based =
on Lawrence's patch. I just put it there to support the discussion:
=
http://www.cubinlab.ee.unimelb.edu.au/~jrid/patches/r228191/bpfffcounter_c=
oncept_v2.patch
Since it is essential to read the hardware counter only once to produce =
the timestamp in second and the ffcounter, the prototype of the =
timestamping function stored in the bpf_d would have to be changed (if =
the kernel is compiled with the FFCLOCK option, at least).
For example, if BPF_T_FFCLOCK is set, this change will make use of =
ffclock_abstime() and solves the performance issue I raised above.
If the clock flag is not set, then the [get]binuptime() will need a =
wrapper function, and they will bear the timestamping latency cost.
Is this acceptable? If yes, then I don't have any objections to the =
patch.
Julien
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4D87C75E-5C49-4D6D-BFD7-4A1E1C2B1C13>