From: Jason Wang <jasowang@redhat.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org, mst@redhat.com, richardcochran@gmail.com,
Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH RFC v2 3/4] virtio-net: support transmit timestamp
Date: Tue, 9 Feb 2021 13:45:11 +0800 [thread overview]
Message-ID: <6bfdf48d-c780-bc65-b0b9-24a33f18827b@redhat.com> (raw)
In-Reply-To: <20210208185558.995292-4-willemdebruijn.kernel@gmail.com>
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.
> + }
> +}
> +
> static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
> {
> unsigned int len;
> @@ -1412,6 +1432,7 @@ static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
> if (likely(!is_xdp_frame(ptr))) {
> struct sk_buff *skb = ptr;
>
> + virtnet_record_tx_tstamp(sq, skb);
> pr_debug("Sent skb %p\n", skb);
>
> bytes += skb->len;
> @@ -1558,7 +1579,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
> struct virtnet_info *vi = sq->vq->vdev->priv;
> - struct virtio_net_hdr_v1_hash *ht;
> + struct virtio_net_hdr_hash_ts *ht;
> int num_sg;
> unsigned hdr_len = vi->hdr_len;
> bool can_push;
> @@ -1567,7 +1588,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>
> can_push = vi->any_header_sg &&
> !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> - !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> + !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len &&
> + !vi->enable_tx_tstamp;
> /* Even if we can, don't push here yet as this would skew
> * csum_start offset below. */
> if (can_push)
> @@ -1588,10 +1610,12 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> u16 report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
> VIRTIO_NET_HASH_REPORT_OTHER;
>
> - ht->hash_value = cpu_to_le32(skb->hash);
> - ht->hash_report = cpu_to_le16(report);
> - ht->hash_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> + ht->hash.value = cpu_to_le32(skb->hash);
> + ht->hash.report = cpu_to_le16(report);
> + ht->hash.flow_state = cpu_to_le16(VIRTIO_NET_HASH_STATE_DEFAULT);
> }
> + if (vi->enable_tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
> + ht->hdr.flags |= VIRTIO_NET_HDR_F_TSTAMP;
>
> sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
> if (can_push) {
> @@ -2307,7 +2331,13 @@ static int virtnet_get_ts_info(struct net_device *dev,
> info->rx_filters = HWTSTAMP_FILTER_NONE;
> }
>
> - info->tx_types = HWTSTAMP_TX_OFF;
> + if (vi->has_tx_tstamp) {
> + info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
> + SOF_TIMESTAMPING_RAW_HARDWARE;
> + info->tx_types = HWTSTAMP_TX_ON;
> + } else {
> + info->tx_types = HWTSTAMP_TX_OFF;
> + }
>
> return 0;
> }
> @@ -2616,7 +2646,8 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> return -EFAULT;
> if (tsconf.flags)
> return -EINVAL;
> - if (tsconf.tx_type != HWTSTAMP_TX_OFF)
> + if (tsconf.tx_type != HWTSTAMP_TX_OFF &&
> + tsconf.tx_type != HWTSTAMP_TX_ON)
> return -ERANGE;
> if (tsconf.rx_filter != HWTSTAMP_FILTER_NONE &&
> tsconf.rx_filter != HWTSTAMP_FILTER_ALL)
> @@ -2627,6 +2658,11 @@ static int virtnet_ioctl_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> else
> vi->enable_rx_tstamp = tsconf.rx_filter == HWTSTAMP_FILTER_ALL;
>
> + if (!vi->has_tx_tstamp)
> + tsconf.tx_type = HWTSTAMP_TX_OFF;
> + else
> + vi->enable_tx_tstamp = tsconf.tx_type == HWTSTAMP_TX_ON;
> +
> if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> return -EFAULT;
>
> @@ -2641,7 +2677,8 @@ static int virtnet_ioctl_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
> tsconf.flags = 0;
> tsconf.rx_filter = vi->enable_rx_tstamp ? HWTSTAMP_FILTER_ALL :
> HWTSTAMP_FILTER_NONE;
> - tsconf.tx_type = HWTSTAMP_TX_OFF;
> + tsconf.tx_type = vi->enable_tx_tstamp ? HWTSTAMP_TX_ON :
> + HWTSTAMP_TX_OFF;
>
> if (copy_to_user(ifr->ifr_data, &tsconf, sizeof(tsconf)))
> return -EFAULT;
> @@ -3178,6 +3215,12 @@ static int virtnet_probe(struct virtio_device *vdev)
> vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> }
>
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP) &&
> + !vring_use_dma_api(vdev)) {
> + vi->has_tx_tstamp = true;
> + vi->hdr_len = sizeof(struct virtio_net_hdr_hash_ts);
> + }
> +
> if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) ||
> virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> vi->any_header_sg = true;
> @@ -3369,7 +3412,7 @@ static struct virtio_device_id id_table[] = {
> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> - VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP
> + VIRTIO_NET_F_TX_HASH, VIRTIO_NET_F_RX_TSTAMP, VIRTIO_NET_F_TX_TSTAMP
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 71e16b53e9c1..cf5d5d1f9b14 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -238,7 +238,7 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
> * unconditionally on data path.
> */
>
> -static bool vring_use_dma_api(struct virtio_device *vdev)
> +bool vring_use_dma_api(struct virtio_device *vdev)
> {
> if (!virtio_has_dma_quirk(vdev))
> return true;
> @@ -257,6 +257,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>
> return false;
> }
> +EXPORT_SYMBOL_GPL(vring_use_dma_api);
>
> size_t virtio_max_dma_size(struct virtio_device *vdev)
> {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 55ea329fe72a..5289e2812e95 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -140,6 +140,7 @@ int virtio_device_freeze(struct virtio_device *dev);
> int virtio_device_restore(struct virtio_device *dev);
> #endif
>
> +bool vring_use_dma_api(struct virtio_device *vdev);
> size_t virtio_max_dma_size(struct virtio_device *vdev);
>
> #define virtio_device_for_each_vq(vdev, vq) \
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index a5c84410cf92..b5d6f0c6cead 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,7 @@
> * Steering */
> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>
> +#define VIRTIO_NET_F_TX_TSTAMP 54 /* Device sends TAI transmit time */
> #define VIRTIO_NET_F_RX_TSTAMP 55 /* Device sends TAI receive time */
I wonder how much value to split into two features.
Thanks
> #define VIRTIO_NET_F_TX_HASH 56 /* Driver sends hash report */
> #define VIRTIO_NET_F_HASH_REPORT 57 /* Supports hash report */
next prev parent reply other threads:[~2021-02-09 5:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 18:55 [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time Willem de Bruijn
2021-02-08 18:55 ` [PATCH RFC v2 1/4] virtio-net: support transmit hash report Willem de Bruijn
2021-02-08 18:55 ` [PATCH RFC v2 2/4] virtio-net: support receive timestamp Willem de Bruijn
2021-02-09 4:13 ` Jason Wang
2021-02-09 13:53 ` Willem de Bruijn
2021-02-10 4:10 ` Jason Wang
2021-02-08 18:55 ` [PATCH RFC v2 3/4] virtio-net: support transmit timestamp Willem de Bruijn
2021-02-09 5:45 ` Jason Wang [this message]
2021-02-09 16:38 ` Michael S. Tsirkin
2021-02-10 2:36 ` Willem de Bruijn
2021-02-10 4:15 ` Jason Wang
2021-02-10 14:40 ` Willem de Bruijn
2021-02-10 4:12 ` Jason Wang
2021-02-09 9:42 ` Michael S. Tsirkin
2021-02-09 14:45 ` Willem de Bruijn
2021-02-08 18:55 ` [PATCH RFC v2 4/4] virtio-net: support future packet transmit time Willem de Bruijn
2021-05-13 22:49 ` [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp, tx-tstamp and tx-time Willem de Bruijn
2021-05-14 7:12 ` Jason Wang
2021-05-14 12:46 ` Willem de Bruijn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6bfdf48d-c780-bc65-b0b9-24a33f18827b@redhat.com \
--to=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=willemb@google.com \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).