diff mbox series

[rfc,3/3] virtio-net: support transmit timestamp

Message ID 20201228162233.2032571-4-willemdebruijn.kernel@gmail.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series virtio-net: add tx-hash, rx-tstamp and tx-tstamp | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 2 maintainers not CCed: davem@davemloft.net kuba@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 63 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Willem de Bruijn Dec. 28, 2020, 4:22 p.m. UTC 
From: Willem de Bruijn <willemb@google.com>

Add optional delivery time (SO_TXTIME) offload for virtio-net.

The Linux TCP/IP stack tries to avoid bursty transmission and network
congestion through pacing: computing an skb delivery time based on
congestion information. Userspace protocol implementations can achieve
the same with SO_TXTIME. This may also reduce scheduling jitter and
improve RTT estimation.

Pacing can be implemented in ETF or FQ qdiscs or offloaded to NIC
hardware. Allow guests to offload for the same reasons.

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        | 24 +++++++++++++++++-------
 include/uapi/linux/virtio_net.h |  1 +
 2 files changed, 18 insertions(+), 7 deletions(-)

Comments

Richard Cochran Dec. 30, 2020, 12:38 p.m. UTC | #1 
On Mon, Dec 28, 2020 at 11:22:33AM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Add optional delivery time (SO_TXTIME) offload for virtio-net.
> 
> The Linux TCP/IP stack tries to avoid bursty transmission and network
> congestion through pacing: computing an skb delivery time based on
> congestion information. Userspace protocol implementations can achieve
> the same with SO_TXTIME. This may also reduce scheduling jitter and
> improve RTT estimation.

This description is clear, but the Subject line is confusing.  It made
me wonder whether this series is somehow about host/guest synchronization
(but your comments do explain that that isn't the case).

How about this instead?

   virtio-net: support future packet transmit time

Thanks,
Richard
Willem de Bruijn Dec. 30, 2020, 3:25 p.m. UTC | #2 
On Wed, Dec 30, 2020 at 7:38 AM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Mon, Dec 28, 2020 at 11:22:33AM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Add optional delivery time (SO_TXTIME) offload for virtio-net.
> >
> > The Linux TCP/IP stack tries to avoid bursty transmission and network
> > congestion through pacing: computing an skb delivery time based on
> > congestion information. Userspace protocol implementations can achieve
> > the same with SO_TXTIME. This may also reduce scheduling jitter and
> > improve RTT estimation.
>
> This description is clear, but the Subject line is confusing.  It made
> me wonder whether this series is somehow about host/guest synchronization
> (but your comments do explain that that isn't the case).
>
> How about this instead?
>
>    virtio-net: support future packet transmit time

Yes, that's clearer. As is, this could easily be mistaken for
SOF_TIMESTAMPING_TX_*, which it is not. Will update, thanks.

Related terms already in use are SO_TXTIME, Earliest Delivery Time
(EDT) and Earliest TxTime First (ETF).

I should probably also s/TX_TSTAMP/TX_TIME/  in the code for the same reason.
kernel test robot Feb. 2, 2021, 1:47 p.m. UTC | #3 
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/be9cab7692382c7886333b498d1d8adbba1881f7
        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 be9cab7692382c7886333b498d1d8adbba1881f7
        # 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@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   drivers/net/virtio_net.c:1099: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:1099:80: sparse:     expected unsigned long long [usertype] ns
   drivers/net/virtio_net.c:1099:80: sparse:     got restricted __virtio64 [usertype] tstamp
>> drivers/net/virtio_net.c:1583:33: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] value @@     got restricted __virtio32 @@
   drivers/net/virtio_net.c:1583:33: sparse:     expected restricted __le32 [usertype] value
   drivers/net/virtio_net.c:1583:33: sparse:     got restricted __virtio32
>> drivers/net/virtio_net.c:1584:34: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] report @@     got int @@
   drivers/net/virtio_net.c:1584:34: sparse:     expected restricted __le16 [usertype] report
   drivers/net/virtio_net.c:1584:34: sparse:     got int

vim +1583 drivers/net/virtio_net.c

  1550	
  1551	static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
  1552	{
  1553		struct virtio_net_hdr_mrg_rxbuf *hdr;
  1554		const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
  1555		struct virtnet_info *vi = sq->vq->vdev->priv;
  1556		struct virtio_net_hdr_v12 *h12;
  1557		int num_sg;
  1558		unsigned hdr_len = vi->hdr_len;
  1559		bool can_push;
  1560	
  1561		pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
  1562	
  1563		can_push = vi->any_header_sg &&
  1564			!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
  1565			!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
  1566		/* Even if we can, don't push here yet as this would skew
  1567		 * csum_start offset below. */
  1568		if (can_push)
  1569			hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
  1570		else
  1571			hdr = skb_vnet_hdr(skb);
  1572	
  1573		if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
  1574					    virtio_is_little_endian(vi->vdev), false,
  1575					    0))
  1576			BUG();
  1577	
  1578		if (vi->mergeable_rx_bufs)
  1579			hdr->num_buffers = 0;
  1580	
  1581		h12 = (void *)hdr;
  1582		if (vi->has_tx_hash) {
> 1583			h12->hash.value = cpu_to_virtio32(vi->vdev, skb->hash);
> 1584			h12->hash.report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
  1585							  VIRTIO_NET_HASH_REPORT_OTHER;
  1586			h12->hash.flow_state = VIRTIO_NET_HASH_STATE_DEFAULT;
  1587		}
  1588		if (vi->has_tx_tstamp)
  1589			h12->tstamp = cpu_to_virtio64(vi->vdev, skb->tstamp);
  1590	
  1591		sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
  1592		if (can_push) {
  1593			__skb_push(skb, hdr_len);
  1594			num_sg = skb_to_sgvec(skb, sq->sg, 0, skb->len);
  1595			if (unlikely(num_sg < 0))
  1596				return num_sg;
  1597			/* Pull header back to avoid skew in tx bytes calculations. */
  1598			__skb_pull(skb, hdr_len);
  1599		} else {
  1600			sg_set_buf(sq->sg, hdr, hdr_len);
  1601			num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len);
  1602			if (unlikely(num_sg < 0))
  1603				return num_sg;
  1604			num_sg++;
  1605		}
  1606		return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
  1607	}
  1608	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 57744bb6a141..d40be688aed0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -207,6 +207,9 @@  struct virtnet_info {
 	/* Host will pass CLOCK_TAI receive time to the guest */
 	bool has_rx_tstamp;
 
+	/* Guest will pass CLOCK_TAI delivery time to the host */
+	bool has_tx_tstamp;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -1550,7 +1553,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_v12 *h12;
 	int num_sg;
 	unsigned hdr_len = vi->hdr_len;
 	bool can_push;
@@ -1575,13 +1578,15 @@  static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 	if (vi->mergeable_rx_bufs)
 		hdr->num_buffers = 0;
 
-	ht = (void *)hdr;
+	h12 = (void *)hdr;
 	if (vi->has_tx_hash) {
-		ht->hash_value = cpu_to_virtio32(vi->vdev, skb->hash);
-		ht->hash_report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
-						 VIRTIO_NET_HASH_REPORT_OTHER;
-		ht->hash_state = VIRTIO_NET_HASH_STATE_DEFAULT;
+		h12->hash.value = cpu_to_virtio32(vi->vdev, skb->hash);
+		h12->hash.report = skb->l4_hash ? VIRTIO_NET_HASH_REPORT_L4 :
+						  VIRTIO_NET_HASH_REPORT_OTHER;
+		h12->hash.flow_state = VIRTIO_NET_HASH_STATE_DEFAULT;
 	}
+	if (vi->has_tx_tstamp)
+		h12->tstamp = cpu_to_virtio64(vi->vdev, skb->tstamp);
 
 	sg_init_table(sq->sg, skb_shinfo(skb)->nr_frags + (can_push ? 1 : 2));
 	if (can_push) {
@@ -3089,6 +3094,11 @@  static int virtnet_probe(struct virtio_device *vdev)
 		vi->hdr_len = sizeof(struct virtio_net_hdr_v12);
 	}
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_TX_TSTAMP)) {
+		vi->has_tx_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;
@@ -3279,7 +3289,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/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 0ffe2eeebd4a..da017a47791d 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	/* Guest sets TAI delivery time */
 #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 */