Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp

[Jason Wang <jasowang@redhat.com>]

On 2021/2/10 上午10:36, Willem de Bruijn wrote:
> On Tue, Feb 9, 2021 at 11:39 AM Michael S. Tsirkin<mst@redhat.com>  wrote:
>> On Tue, Feb 09, 2021 at 01:45:11PM +0800, Jason Wang wrote:
>>> On 2021/2/9 上午2:55, Willem de Bruijn wrote:
>>>> From: Willem de Bruijn<willemb@google.com>
>>>>
>>>> Add optional PTP hardware tx timestamp offload for virtio-net.
>>>>
>>>> Accurate RTT measurement requires timestamps close to the wire.
>>>> Introduce virtio feature VIRTIO_NET_F_TX_TSTAMP, the transmit
>>>> equivalent to VIRTIO_NET_F_RX_TSTAMP.
>>>>
>>>> The driver sets VIRTIO_NET_HDR_F_TSTAMP to request a timestamp
>>>> returned on completion. If the feature is negotiated, the device
>>>> either places the timestamp or clears the feature bit.
>>>>
>>>> The timestamp straddles (virtual) hardware domains. Like PTP, use
>>>> international atomic time (CLOCK_TAI) as global clock base. The driver
>>>> must sync with the device, e.g., through kvm-clock.
>>>>
>>>> Modify can_push to ensure that on tx completion the header, and thus
>>>> timestamp, is in a predicatable location at skb_vnet_hdr.
>>>>
>>>> RFC: this implementation relies on the device writing to the buffer.
>>>> That breaks DMA_TO_DEVICE semantics. For now, disable when DMA is on.
>>>> The virtio changes should be a separate patch at the least.
>>>>
>>>> Tested: modified txtimestamp.c to with h/w timestamping:
>>>>     -       sock_opt = SOF_TIMESTAMPING_SOFTWARE |
>>>>     +       sock_opt = SOF_TIMESTAMPING_RAW_HARDWARE |
>>>>     + do_test(family, SOF_TIMESTAMPING_TX_HARDWARE);
>>>>
>>>> Signed-off-by: Willem de Bruijn<willemb@google.com>
>>>> ---
>>>>    drivers/net/virtio_net.c        | 61 ++++++++++++++++++++++++++++-----
>>>>    drivers/virtio/virtio_ring.c    |  3 +-
>>>>    include/linux/virtio.h          |  1 +
>>>>    include/uapi/linux/virtio_net.h |  1 +
>>>>    4 files changed, 56 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index ac44c5efa0bc..fc8ecd3a333a 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -210,6 +210,12 @@ struct virtnet_info {
>>>>      /* Device will pass rx timestamp. Requires has_rx_tstamp */
>>>>      bool enable_rx_tstamp;
>>>> +   /* Device can pass CLOCK_TAI transmit time to the driver */
>>>> +   bool has_tx_tstamp;
>>>> +
>>>> +   /* Device will pass tx timestamp. Requires has_tx_tstamp */
>>>> +   bool enable_tx_tstamp;
>>>> +
>>>>      /* Has control virtqueue */
>>>>      bool has_cvq;
>>>> @@ -1401,6 +1407,20 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>>>>      return stats.packets;
>>>>    }
>>>> +static void virtnet_record_tx_tstamp(const struct send_queue *sq,
>>>> +                                struct sk_buff *skb)
>>>> +{
>>>> +   const struct virtio_net_hdr_hash_ts *h = skb_vnet_hdr_ht(skb);
>>>> +   const struct virtnet_info *vi = sq->vq->vdev->priv;
>>>> +   struct skb_shared_hwtstamps ts;
>>>> +
>>>> +   if (h->hdr.flags & VIRTIO_NET_HDR_F_TSTAMP &&
>>>> +       vi->enable_tx_tstamp) {
>>>> +           ts.hwtstamp = ns_to_ktime(le64_to_cpu(h->tstamp));
>>>> +           skb_tstamp_tx(skb, &ts);
>>> This probably won't work since the buffer is read-only from the device. (See
>>> virtqueue_add_outbuf()).
>>>
>>> Another issue that I vaguely remember that the virtio spec forbids out
>>> buffer after in buffer.
>> Both Driver Requirements: Message Framing and Driver Requirements: Scatter-Gather Support
>> have this statement:
>>
>>          The driver MUST place any device-writable descriptor elements after any device-readable descriptor ele-
>>          ments.
>>
>>
>> similarly
>>
>> Device Requirements: The Virtqueue Descriptor Table
>>          A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable
>>          buffer.
> Thanks. That's clear. So the clean solution would be to add a
> device-writable descriptor after the existing device-readable ones.


I think so, but a question is the format for this tailer. I think it 
might be better to post a spec patch to discuss.

Thanks


>
> And the device must be aware that this is to return the tstamp only.
> In the example implementation of vhost, it has to exclude this last
> descriptor from the msg->msg_iter iovec array with packet data
> initialized at get_tx_bufs/init_iov_iter.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization