On Tue, Nov 28, 2017 at 11:28 PM, Willem de Bruijn
<willemdebruijn.ker...@gmail.com> wrote:
> On Tue, Nov 28, 2017 at 3:32 PM, Arnd Bergmann <a...@arndb.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.ker...@gmail.com>
>> Signed-off-by: Arnd Bergmann <a...@arndb.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
