Re: [PATCH] [RFC v3] packet: experimental support for 64-bit timestamps

[Willem de Bruijn]

On Wed, Nov 29, 2017 at 3:06 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wed, Nov 29, 2017 at 5:51 PM, Willem de Bruijn
> <willemdebruijn.kernel@xxxxxxxxx> wrote:
>>> 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.
>>
>> I could not find any good tests for this interface, either. The only
>> user of the interface I found was a little tool I wrote a few years
>> ago that compares timestamps at multiple points in the transmit
>> path for latency measurement [1]. But it may be easier to just write
>> a new test under tools/testing/selftests/net for this purpose. I can
>> help with that, too, if you want.
>
> Thanks, that would be great!

I'll reply to this thread with git send-email with an extension to
tools/testing/selftests/net/psock_tpacket.c. I can resend that for
submission after your feature is merged (as it depends on it) or
feel free to include it in your patchset. The test currently fails for
the ns64 case. I probably did not convert correctly, but have to leave
the office and want to send what I have.

Two other comments: the test crashed the kernel due to a NULL ptr
in tpacket_get_timestamp. We cannot rely on skb->sk being set to
the packet socket here. And assignment to bitfield requires a cast to
boolean.

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f55f330ab547..e9decc7fc5c3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -439,9 +439,9 @@ static int __packet_get_status(struct packet_sock
*po, void *frame)
        }
 }

-static __u32 tpacket_get_timestamp(struct sk_buff *skb, __u32 *hi, __u32 *lo)
+static __u32 tpacket_get_timestamp(struct packet_sock *po, struct sk_buff *skb,
+                                  __u32 *hi, __u32 *lo)
 {
-       struct packet_sock *po = pkt_sk(skb->sk);
        struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
        ktime_t stamp;
        u32 type;
@@ -508,7 +508,7 @@ static __u32 __packet_set_timestamp(struct
packet_sock *po, void *frame,
        union tpacket_uhdr h;
        __u32 ts_status, hi, lo;

-       if (!(ts_status = tpacket_get_timestamp(skb, &hi, &lo)))
+       if (!(ts_status = tpacket_get_timestamp(po, skb, &hi, &lo)))
                return 0;

        h.raw = frame;
@@ -2352,7 +2352,7 @@ static int tpacket_rcv(struct sk_buff *skb,
struct net_device *dev,

        skb_copy_bits(skb, 0, h.raw + macoff, snaplen);

-       if (!(ts_status = tpacket_get_timestamp(skb, &tstamp_hi, &tstamp_lo)))
+       if (!(ts_status = tpacket_get_timestamp(po, skb, &tstamp_hi,
&tstamp_lo)))
                packet_get_time(po, &tstamp_hi, &tstamp_lo);

        status |= ts_status;
@@ -3835,7 +3835,7 @@ packet_setsockopt(struct socket *sock, int
level, int optname, char __user *optv
                if (copy_from_user(&val, optval, sizeof(val)))
                        return -EFAULT;

-               po->tp_skiptstamp = val;
+               po->tp_skiptstamp = !!val;
                return 0;
        }
        case PACKET_TIMESTAMP_NS64:
@@ -3847,7 +3847,7 @@ packet_setsockopt(struct socket *sock, int
level, int optname, char __user *optv
                if (copy_from_user(&val, optval, sizeof(val)))
                        return -EFAULT;

-               po->tp_tstamp_ns64 = val;
+               po->tp_tstamp_ns64 = !!val;
                return 0;
        }
        case PACKET_FANOUT: