Willem de Bruijn
2020-Dec-28 16:22 UTC
[PATCH rfc 2/3] virtio-net: support receive timestamp
From: Willem de Bruijn <willemb at 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 at 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;
+ __virtio64 tstamp;
+};
+
#ifndef VIRTIO_NET_NO_LEGACY
/* This header comes first in the scatter-gather list.
* For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
--
2.29.2.729.g45daf8777d-goog
Michael S. Tsirkin
2020-Dec-28 17:28 UTC
[PATCH rfc 2/3] virtio-net: support receive timestamp
On Mon, Dec 28, 2020 at 11:22:32AM -0500, Willem de Bruijn wrote:> From: Willem de Bruijn <willemb at 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 at 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; > + __virtio64 tstamp; > +}; > + > #ifndef VIRTIO_NET_NO_LEGACY > /* This header comes first in the scatter-gather list. > * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it mustSo it looks like VIRTIO_NET_F_RX_TSTAMP should depend on both VIRTIO_NET_F_RX_TSTAMP and VIRTIO_NET_F_HASH_REPORT then? I am not sure what does v12 mean here. virtio_net_hdr_v1 is just with VIRTIO_F_VERSION_1, virtio_net_hdr_v1_hash is with VIRTIO_F_VERSION_1 and VIRTIO_NET_F_HASH_REPORT. So this one is virtio_net_hdr_hash_tstamp I guess?> -- > 2.29.2.729.g45daf8777d-goog
kernel test robot
2021-Feb-02 13:05 UTC
[PATCH rfc 2/3] virtio-net: support receive timestamp
Hi Willem,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on ipvs/master]
[also build test WARNING on linus/master v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/virtio-net-add-tx-hash-rx-tstamp-and-tx-tstamp/20201229-002604
base: https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master
config: x86_64-randconfig-s021-20201228 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-215-g0fb77bb6-dirty
#
https://github.com/0day-ci/linux/commit/d309db6857fa35b0d7a11cc5229436d6d71ab274
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Willem-de-Bruijn/virtio-net-add-tx-hash-rx-tstamp-and-tx-tstamp/20201229-002604
git checkout d309db6857fa35b0d7a11cc5229436d6d71ab274
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp at intel.com>
"sparse warnings: (new ones prefixed by
>>)">> drivers/net/virtio_net.c:1096:80: sparse: sparse: incorrect type in
argument 1 (different base types) @@ expected unsigned long long [usertype]
ns @@ got restricted __virtio64 [usertype] tstamp @@
drivers/net/virtio_net.c:1096:80: sparse: expected unsigned long long
[usertype] ns
drivers/net/virtio_net.c:1096:80: sparse: got restricted __virtio64
[usertype] tstamp
drivers/net/virtio_net.c:1580:32: sparse: sparse: incorrect type in
assignment (different base types) @@ expected restricted __le32 [usertype]
hash_value @@ got restricted __virtio32 @@
drivers/net/virtio_net.c:1580:32: sparse: expected restricted __le32
[usertype] hash_value
drivers/net/virtio_net.c:1580:32: sparse: got restricted __virtio32
drivers/net/virtio_net.c:1581:33: sparse: sparse: incorrect type in
assignment (different base types) @@ expected restricted __le16 [usertype]
hash_report @@ got int @@
drivers/net/virtio_net.c:1581:33: sparse: expected restricted __le16
[usertype] hash_report
drivers/net/virtio_net.c:1581:33: sparse: got int
vim +1096 drivers/net/virtio_net.c
1048
1049 static void receive_buf(struct virtnet_info *vi, struct receive_queue
*rq,
1050 void *buf, unsigned int len, void **ctx,
1051 unsigned int *xdp_xmit,
1052 struct virtnet_rq_stats *stats)
1053 {
1054 struct net_device *dev = vi->dev;
1055 struct sk_buff *skb;
1056 struct virtio_net_hdr_mrg_rxbuf *hdr;
1057
1058 if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
1059 pr_debug("%s: short packet %i\n", dev->name, len);
1060 dev->stats.rx_length_errors++;
1061 if (vi->mergeable_rx_bufs) {
1062 put_page(virt_to_head_page(buf));
1063 } else if (vi->big_packets) {
1064 give_pages(rq, buf);
1065 } else {
1066 put_page(virt_to_head_page(buf));
1067 }
1068 return;
1069 }
1070
1071 if (vi->mergeable_rx_bufs)
1072 skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
1073 stats);
1074 else if (vi->big_packets)
1075 skb = receive_big(dev, vi, rq, buf, len, stats);
1076 else
1077 skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
1078
1079 if (unlikely(!skb))
1080 return;
1081
1082 hdr = skb_vnet_hdr(skb);
1083
1084 if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
1085 skb->ip_summed = CHECKSUM_UNNECESSARY;
1086
1087 if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
1088 virtio_is_little_endian(vi->vdev))) {
1089 net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
1090 dev->name, hdr->hdr.gso_type,
1091 hdr->hdr.gso_size);
1092 goto frame_err;
1093 }
1094
1095 if (vi->has_rx_tstamp)> 1096 skb_hwtstamps(skb)->hwtstamp =
ns_to_ktime(skb_vnet_hdr_12(skb)->tstamp);
1097
1098 skb_record_rx_queue(skb, vq2rxq(rq->vq));
1099 skb->protocol = eth_type_trans(skb, dev);
1100 pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
1101 ntohs(skb->protocol), skb->len, skb->pkt_type);
1102
1103 napi_gro_receive(&rq->napi, skb);
1104 return;
1105
1106 frame_err:
1107 dev->stats.rx_frame_errors++;
1108 dev_kfree_skb(skb);
1109 }
1110
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all at lists.01.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 34123 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210202/e0e158ef/attachment-0001.gz>
Michael S. Tsirkin
2021-Feb-02 14:08 UTC
[PATCH rfc 2/3] virtio-net: support receive timestamp
On Mon, Dec 28, 2020 at 11:22:32AM -0500, Willem de Bruijn wrote:> From: Willem de Bruijn <willemb at 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 at 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?> + __virtio64 tstamp; > +}; > +Given it's only available in modern devices, I think we can make this __le64 tstamp.> #ifndef VIRTIO_NET_NO_LEGACY > /* This header comes first in the scatter-gather list. > * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must > -- > 2.29.2.729.g45daf8777d-goog