From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>, virtualization@lists.linux-foundation.org, Network Development <netdev@vger.kernel.org>, Jason Wang <jasowang@redhat.com> Subject: Re: [PATCH rfc 2/3] virtio-net: support receive timestamp Date: Tue, 2 Feb 2021 18:43:59 -0500 [thread overview] Message-ID: <CA+FuTSfRHMDd-Q4UB4vVsdbs=YpP-WzUMtNGiKwLpEQaAR2Xdg@mail.gmail.com> (raw) In-Reply-To: <20210202175934-mutt-send-email-mst@kernel.org> On Tue, Feb 2, 2021 at 6:06 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Feb 02, 2021 at 05:17:13PM -0500, Willem de Bruijn wrote: > > On Tue, Feb 2, 2021 at 9:08 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Dec 28, 2020 at 11:22:32AM -0500, Willem de Bruijn wrote: > > > > From: Willem de Bruijn <willemb@google.com> > > > > > > > > Add optional PTP hardware timestamp offload for virtio-net. > > > > > > > > Accurate RTT measurement requires timestamps close to the wire. > > > > Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the > > > > virtio-net header is expanded with room for a timestamp. A host may > > > > pass receive timestamps for all or some packets. A timestamp is valid > > > > if non-zero. > > > > > > > > The timestamp straddles (virtual) hardware domains. Like PTP, use > > > > international atomic time (CLOCK_TAI) as global clock base. It is > > > > guest responsibility to sync with host, e.g., through kvm-clock. > > > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > > > --- > > > > drivers/net/virtio_net.c | 20 +++++++++++++++++++- > > > > include/uapi/linux/virtio_net.h | 12 ++++++++++++ > > > > 2 files changed, 31 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index b917b7333928..57744bb6a141 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -204,6 +204,9 @@ struct virtnet_info { > > > > /* Guest will pass tx path info to the host */ > > > > bool has_tx_hash; > > > > > > > > + /* Host will pass CLOCK_TAI receive time to the guest */ > > > > + bool has_rx_tstamp; > > > > + > > > > /* Has control virtqueue */ > > > > bool has_cvq; > > > > > > > > @@ -292,6 +295,13 @@ static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff *skb) > > > > return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb; > > > > } > > > > > > > > +static inline struct virtio_net_hdr_v12 *skb_vnet_hdr_12(struct sk_buff *skb) > > > > +{ > > > > + BUILD_BUG_ON(sizeof(struct virtio_net_hdr_v12) > sizeof(skb->cb)); > > > > + > > > > + return (void *)skb->cb; > > > > +} > > > > + > > > > /* > > > > * private is used to chain pages for big packets, put the whole > > > > * most recent used list in the beginning for reuse > > > > @@ -1082,6 +1092,9 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > goto frame_err; > > > > } > > > > > > > > + if (vi->has_rx_tstamp) > > > > + skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(skb_vnet_hdr_12(skb)->tstamp); > > > > + > > > > skb_record_rx_queue(skb, vq2rxq(rq->vq)); > > > > skb->protocol = eth_type_trans(skb, dev); > > > > pr_debug("Receiving skb proto 0x%04x len %i type %i\n", > > > > @@ -3071,6 +3084,11 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash); > > > > } > > > > > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) { > > > > + vi->has_rx_tstamp = true; > > > > + vi->hdr_len = sizeof(struct virtio_net_hdr_v12); > > > > + } > > > > + > > > > if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) || > > > > virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) > > > > vi->any_header_sg = true; > > > > @@ -3261,7 +3279,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_TX_HASH, VIRTIO_NET_F_RX_TSTAMP > > > > > > > > static unsigned int features[] = { > > > > VIRTNET_FEATURES, > > > > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > > > > index f6881b5b77ee..0ffe2eeebd4a 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_RX_TSTAMP 55 /* Host sends TAI receive time */ > > > > #define VIRTIO_NET_F_TX_HASH 56 /* Guest sends hash report */ > > > > #define VIRTIO_NET_F_HASH_REPORT 57 /* Supports hash report */ > > > > #define VIRTIO_NET_F_RSS 60 /* Supports RSS RX steering */ > > > > @@ -182,6 +183,17 @@ struct virtio_net_hdr_v1_hash { > > > > }; > > > > }; > > > > > > > > +struct virtio_net_hdr_v12 { > > > > + struct virtio_net_hdr_v1 hdr; > > > > + struct { > > > > + __le32 value; > > > > + __le16 report; > > > > + __le16 flow_state; > > > > + } hash; > > > > + __virtio32 reserved; > > > > > > > > > Does endian-ness matter? If not - just u32? > > > > I suppose it does not matter as long as this is reserved. Should it be > > __le32, at least? > > One can safely assign 0 to any value. Ack. > > > > > + __virtio64 tstamp; > > > > +}; > > > > + > > > > > > Given it's only available in modern devices, I think we > > > can make this __le64 tstamp. > > > > Actually, would it be possible to make new features available on > > legacy devices? There is nothing in the features bits precluding it. > > I think it won't be possible: you are using feature bit 55, > legacy devices have up to 32 feature bits. And of course the > header looks a bit differently for legacy, you would have to add special > code to handle that when mergeable buffers are off. I think I can make the latter work. I did start without a dependency on the v1 header initially. Feature bit array length I had not considered. Good point. Need to think about that. It would be very appealing if in particular the tx-hash feature could work in legacy mode. > > > > I have a revised patchset almost ready. I suppose I should send it as > > RFC again, and simultaneously file an OASIS ballot for each feature? > > that would be great. Will do, thanks.
WARNING: multiple messages have this Message-ID (diff)
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: Network Development <netdev@vger.kernel.org>, Willem de Bruijn <willemdebruijn.kernel@gmail.com>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH rfc 2/3] virtio-net: support receive timestamp Date: Tue, 2 Feb 2021 18:43:59 -0500 [thread overview] Message-ID: <CA+FuTSfRHMDd-Q4UB4vVsdbs=YpP-WzUMtNGiKwLpEQaAR2Xdg@mail.gmail.com> (raw) In-Reply-To: <20210202175934-mutt-send-email-mst@kernel.org> On Tue, Feb 2, 2021 at 6:06 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Feb 02, 2021 at 05:17:13PM -0500, Willem de Bruijn wrote: > > On Tue, Feb 2, 2021 at 9:08 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Dec 28, 2020 at 11:22:32AM -0500, Willem de Bruijn wrote: > > > > From: Willem de Bruijn <willemb@google.com> > > > > > > > > Add optional PTP hardware timestamp offload for virtio-net. > > > > > > > > Accurate RTT measurement requires timestamps close to the wire. > > > > Introduce virtio feature VIRTIO_NET_F_RX_TSTAMP. If negotiated, the > > > > virtio-net header is expanded with room for a timestamp. A host may > > > > pass receive timestamps for all or some packets. A timestamp is valid > > > > if non-zero. > > > > > > > > The timestamp straddles (virtual) hardware domains. Like PTP, use > > > > international atomic time (CLOCK_TAI) as global clock base. It is > > > > guest responsibility to sync with host, e.g., through kvm-clock. > > > > > > > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > > > --- > > > > drivers/net/virtio_net.c | 20 +++++++++++++++++++- > > > > include/uapi/linux/virtio_net.h | 12 ++++++++++++ > > > > 2 files changed, 31 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index b917b7333928..57744bb6a141 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -204,6 +204,9 @@ struct virtnet_info { > > > > /* Guest will pass tx path info to the host */ > > > > bool has_tx_hash; > > > > > > > > + /* Host will pass CLOCK_TAI receive time to the guest */ > > > > + bool has_rx_tstamp; > > > > + > > > > /* Has control virtqueue */ > > > > bool has_cvq; > > > > > > > > @@ -292,6 +295,13 @@ static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff *skb) > > > > return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb; > > > > } > > > > > > > > +static inline struct virtio_net_hdr_v12 *skb_vnet_hdr_12(struct sk_buff *skb) > > > > +{ > > > > + BUILD_BUG_ON(sizeof(struct virtio_net_hdr_v12) > sizeof(skb->cb)); > > > > + > > > > + return (void *)skb->cb; > > > > +} > > > > + > > > > /* > > > > * private is used to chain pages for big packets, put the whole > > > > * most recent used list in the beginning for reuse > > > > @@ -1082,6 +1092,9 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > > > > goto frame_err; > > > > } > > > > > > > > + if (vi->has_rx_tstamp) > > > > + skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(skb_vnet_hdr_12(skb)->tstamp); > > > > + > > > > skb_record_rx_queue(skb, vq2rxq(rq->vq)); > > > > skb->protocol = eth_type_trans(skb, dev); > > > > pr_debug("Receiving skb proto 0x%04x len %i type %i\n", > > > > @@ -3071,6 +3084,11 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash); > > > > } > > > > > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_RX_TSTAMP)) { > > > > + vi->has_rx_tstamp = true; > > > > + vi->hdr_len = sizeof(struct virtio_net_hdr_v12); > > > > + } > > > > + > > > > if (virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT) || > > > > virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) > > > > vi->any_header_sg = true; > > > > @@ -3261,7 +3279,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_TX_HASH, VIRTIO_NET_F_RX_TSTAMP > > > > > > > > static unsigned int features[] = { > > > > VIRTNET_FEATURES, > > > > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > > > > index f6881b5b77ee..0ffe2eeebd4a 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_RX_TSTAMP 55 /* Host sends TAI receive time */ > > > > #define VIRTIO_NET_F_TX_HASH 56 /* Guest sends hash report */ > > > > #define VIRTIO_NET_F_HASH_REPORT 57 /* Supports hash report */ > > > > #define VIRTIO_NET_F_RSS 60 /* Supports RSS RX steering */ > > > > @@ -182,6 +183,17 @@ struct virtio_net_hdr_v1_hash { > > > > }; > > > > }; > > > > > > > > +struct virtio_net_hdr_v12 { > > > > + struct virtio_net_hdr_v1 hdr; > > > > + struct { > > > > + __le32 value; > > > > + __le16 report; > > > > + __le16 flow_state; > > > > + } hash; > > > > + __virtio32 reserved; > > > > > > > > > Does endian-ness matter? If not - just u32? > > > > I suppose it does not matter as long as this is reserved. Should it be > > __le32, at least? > > One can safely assign 0 to any value. Ack. > > > > > + __virtio64 tstamp; > > > > +}; > > > > + > > > > > > Given it's only available in modern devices, I think we > > > can make this __le64 tstamp. > > > > Actually, would it be possible to make new features available on > > legacy devices? There is nothing in the features bits precluding it. > > I think it won't be possible: you are using feature bit 55, > legacy devices have up to 32 feature bits. And of course the > header looks a bit differently for legacy, you would have to add special > code to handle that when mergeable buffers are off. I think I can make the latter work. I did start without a dependency on the v1 header initially. Feature bit array length I had not considered. Good point. Need to think about that. It would be very appealing if in particular the tx-hash feature could work in legacy mode. > > > > I have a revised patchset almost ready. I suppose I should send it as > > RFC again, and simultaneously file an OASIS ballot for each feature? > > that would be great. Will do, thanks. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-02-02 23:45 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-28 16:22 [PATCH rfc 0/3] virtio-net: add tx-hash, rx-tstamp and tx-tstamp Willem de Bruijn 2020-12-28 16:22 ` Willem de Bruijn 2020-12-28 16:22 ` [PATCH rfc 1/3] virtio-net: support transmit hash report Willem de Bruijn 2020-12-28 16:22 ` Willem de Bruijn 2020-12-28 16:28 ` Michael S. Tsirkin 2020-12-28 16:28 ` Michael S. Tsirkin 2020-12-28 16:47 ` Willem de Bruijn 2020-12-28 16:47 ` Willem de Bruijn 2020-12-28 17:22 ` Michael S. Tsirkin 2020-12-28 17:22 ` Michael S. Tsirkin 2020-12-29 1:19 ` Willem de Bruijn 2020-12-29 1:19 ` Willem de Bruijn 2020-12-28 21:36 ` Michael S. Tsirkin 2020-12-28 21:36 ` Michael S. Tsirkin 2020-12-29 1:23 ` Willem de Bruijn 2020-12-29 1:23 ` Willem de Bruijn 2020-12-28 16:22 ` [PATCH rfc 2/3] virtio-net: support receive timestamp Willem de Bruijn 2020-12-28 16:22 ` Willem de Bruijn 2020-12-28 17:28 ` Michael S. Tsirkin 2020-12-28 17:28 ` Michael S. Tsirkin 2020-12-28 19:30 ` Willem de Bruijn 2020-12-28 19:30 ` Willem de Bruijn 2020-12-28 21:32 ` Michael S. Tsirkin 2020-12-28 21:32 ` Michael S. Tsirkin 2020-12-29 1:05 ` Willem de Bruijn 2020-12-29 1:05 ` Willem de Bruijn 2020-12-29 9:17 ` Jason Wang 2020-12-29 9:17 ` Jason Wang 2020-12-29 14:20 ` Willem de Bruijn 2020-12-29 14:20 ` Willem de Bruijn 2020-12-30 8:38 ` Jason Wang 2020-12-30 8:38 ` Jason Wang 2020-12-28 22:59 ` Jakub Kicinski 2020-12-29 0:57 ` Willem de Bruijn 2020-12-29 0:57 ` Willem de Bruijn 2020-12-30 8:44 ` Jason Wang 2020-12-30 8:44 ` Jason Wang 2020-12-30 12:30 ` Richard Cochran 2021-02-02 13:05 ` kernel test robot 2021-02-02 13:05 ` kernel test robot 2021-02-02 13:05 ` kernel test robot 2021-02-02 14:08 ` Michael S. Tsirkin 2021-02-02 14:08 ` Michael S. Tsirkin 2021-02-02 22:17 ` Willem de Bruijn 2021-02-02 22:17 ` Willem de Bruijn 2021-02-02 23:02 ` Michael S. Tsirkin 2021-02-02 23:02 ` Michael S. Tsirkin 2021-02-02 23:43 ` Willem de Bruijn [this message] 2021-02-02 23:43 ` Willem de Bruijn 2020-12-28 16:22 ` [PATCH rfc 3/3] virtio-net: support transmit timestamp Willem de Bruijn 2020-12-28 16:22 ` Willem de Bruijn 2020-12-30 12:38 ` Richard Cochran 2020-12-30 15:25 ` Willem de Bruijn 2020-12-30 15:25 ` Willem de Bruijn 2021-02-02 13:47 ` kernel test robot 2021-02-02 13:47 ` kernel test robot 2021-02-02 13:47 ` kernel test robot 2020-12-28 17:29 ` [PATCH rfc 0/3] virtio-net: add tx-hash, rx-tstamp and tx-tstamp Michael S. Tsirkin 2020-12-28 17:29 ` Michael S. Tsirkin 2020-12-28 19:51 ` Willem de Bruijn 2020-12-28 19:51 ` Willem de Bruijn 2020-12-28 21:38 ` Michael S. Tsirkin 2020-12-28 21:38 ` Michael S. Tsirkin 2020-12-29 1:14 ` Willem de Bruijn 2020-12-29 1:14 ` Willem de Bruijn 2021-01-06 20:32 ` Willem de Bruijn 2021-01-06 20:32 ` 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='CA+FuTSfRHMDd-Q4UB4vVsdbs=YpP-WzUMtNGiKwLpEQaAR2Xdg@mail.gmail.com' \ --to=willemdebruijn.kernel@gmail.com \ --cc=jasowang@redhat.com \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.