Date:   Wed, 29 Nov 2017 13:31:01 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Björn Töpel <bjorn.topel@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Mike Maloney <maloney@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        "Rosen, Rami" <rami.rosen@...el.com>,
        "Reshetova, Elena" <elena.reshetova@...el.com>,
        Sowmini Varadhan <sowmini.varadhan@...cle.com>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFC v3] packet: experimental support for 64-bit timestamps

On Tue, Nov 28, 2017 at 11:28 PM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Tue, Nov 28, 2017 at 3:32 PM, Arnd Bergmann <arnd@...db.de> wrote:
>> As I noticed in my previous patch to remove the 'timespec' usage in
>> the packet socket, the timestamps in the packet socket are slightly
>> inefficient as they convert a nanosecond value into seconds/nanoseconds
>> or seconds/microseconds.
>>
>> This adds two new socket options for the timestamp to resolve that:
>>
>> PACKET_SKIPTIMESTAMP sets a flag to indicate whether to generate
>> timestamps at all. When this is set, all timestamps are hardcoded to
>> zero, which saves a few cycles for the conversion and the access of
>> the hardware clocksource. The idea was taken from pktgen, which has an
>> F_NO_TIMESTAMP option for the same purpose.
>>
>> PACKET_TIMESTAMP_NS64 changes the interpretation of the time stamp fields:
>> instead of having 32 bits for seconds plus 32 bits for nanoseconds or
>> microseconds, we now always send down 64 bits worth of nanoseconds when
>> this flag is set.
>>
>> Link: https://patchwork.kernel.org/patch/10077199/
>> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@...il.com>
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>
> This works. Another option would be to add a PACKET_TIMESTAMP_EX
> with the semantics we discussed previously + fail hard when any undefined
> bits are set. I don't feel strong either way, we don't intend to extend further.
>
> If taking this approach, it might be good to split into separate patches, one
> for each flag?
>
>> -static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec64 *ts,
>> +static __u32 tpacket_get_timestamp(struct sk_buff *skb, __u32 *hi, __u32 *lo,
>>                                    unsigned int flags)
>
> Argument flags is no longer used.

Fixed

>> -       return 0;
>> +               *hi = upper_32_bits(ns);
>> +               *lo = lower_32_bits(ns);
>> +       } else {
>> +               struct timespec64 ts = ktime_to_timespec64(stamp);
>> +
>> +               *hi = ts.tv_sec;
>> +               if (po->tp_version == TPACKET_V1)
>
> Very minor: may want to invert test to make newer the protocols the
> likely branch.

Ok. I didn't think this would make any difference to the compiler, but
for readability it seems at least as good, so I've changed it as you suggested
and use "po->tp_version > TPACKET_V1".

>>  static __u32 __packet_set_timestamp(struct packet_sock *po, void *frame,
>>                                     struct sk_buff *skb)
>>  {
>>         union tpacket_uhdr h;
>> -       struct timespec64 ts;
>> -       __u32 ts_status;
>> +       __u32 ts_status, hi, lo;
>>
>> -       if (!(ts_status = tpacket_get_timestamp(skb, &ts, po->tp_tstamp)))
>> +       if (!(ts_status = tpacket_get_timestamp(skb, &hi, &lo, po->tp_tstamp)))
>>                 return 0;
>>
>>         h.raw = frame;
>> -       /*
>> -        * versions 1 through 3 overflow the timestamps in y2106, since they
>> -        * all store the seconds in a 32-bit unsigned integer.
>> -        * If we create a version 4, that should have a 64-bit timestamp,
>> -        * either 64-bit seconds + 32-bit nanoseconds, or just 64-bit
>> -        * nanoseconds.
>> -        */
>
> Probably no need to introduce this in patch 1/2 when removing it in 2/2.

I'm still considering this patch as experimental, since I haven't done any
actual testing on it, so I'm not sure it gets merged at the same time.
If patch 1 gets merged separately, I'd rather keep the comment in place.

>> @@ -2191,8 +2226,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>>         unsigned long status = TP_STATUS_USER;
>>         unsigned short macoff, netoff, hdrlen;
>>         struct sk_buff *copy_skb = NULL;
>> -       struct timespec64 ts;
>>         __u32 ts_status;
>> +       __u32 hi, lo;
>
> since this function is not time-specific, the context of hi and lo is not
> immediately obvious here. tstamp_hi, tstamp_lo? Or even __u32
> tstamp[2] and have tpacket_get_timestamp and packet_get_time take
> one fewer argument.

Fixed.

Thanks for the review! Any suggestions for how to do the testing? If you have
existing test cases, could you give my next version a test run to see if there
are any regressions and if the timestamps work as expected?

I see that there are test cases in tools/testing/selftests/net/, but none
of them seem to use the time stamps so far, and I'm not overly familiar
with how it works in the details to extend it in a meaningful way.

        arnd

Powered by blists - more mailing lists