[net PATCH v4 0/6] virtio_net XDP fixes and adjust_header support

STALE3522d

Revision v4 of 4 in this series.

14 messages, 3 authors, 2017-01-17 · open the first message on its own page

[net PATCH v4 0/6] virtio_net XDP fixes and adjust_header support

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-15 23:59:20

This has a fix to handle small buffer free logic correctly and then
also adds adjust head support.

I pushed adjust head at net (even though its rc3) to avoid having
to push another exception case into virtio_net to catch if the
program uses adjust_head and then block it. If there are any strong
objections to this we can push it at net-next and use a patch from
Jakub to add the exception handling but then user space has to deal
with it either via try/fail logic or via kernel version checks. Granted
we already have some cases that need to be configured to enable XDP
but I don't see any reason to have yet another one when we can fix it
now vs delaying a kernel version.


v2: fix spelling error, convert unsigned -> unsigned int
v3: v2 git crashed during send so retrying sorry for the noise
v4: changed layout of rtnl_lock fixes (Stephen)
    moved reset logic into virtio core with new patch (MST)
    fixed up linearize and some code cleanup (Jason)

    Otherwise did some generic code cleanup so might be a bit
    cleaner this time at least that is the hope.

Thanks everyone for the v3 review.

---

John Fastabend (6):
      virtio_net: use dev_kfree_skb for small buffer XDP receive
      virtio_net: wrap rtnl_lock in test for calling with lock already held
      virtio_net: factor out xdp handler for readability
      virtio_net: remove duplicate queue pair binding in XDP
      virtio: add pci_down/pci_up configuration
      virtio_net: XDP support for adjust_head


 drivers/net/virtio_net.c        |  228 ++++++++++++++++++++++++---------------
 drivers/virtio/virtio.c         |   14 ++
 drivers/virtio/virtio_balloon.c |    4 -
 drivers/virtio/virtio_input.c   |    4 -
 include/linux/virtio.h          |    5 -
 5 files changed, 152 insertions(+), 103 deletions(-)

[net PATCH v4 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-15 23:59:44

In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.

Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
---
 drivers/net/virtio_net.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4a10500..d97bb71 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -1890,8 +1890,12 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 			put_page(vi->rq[i].alloc_frag.page);
 }
 
-static bool is_xdp_queue(struct virtnet_info *vi, int q)
+static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
 {
+	/* For small receive mode always use kfree_skb variants */
+	if (!vi->mergeable_rx_bufs)
+		return false;
+
 	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
 		return false;
 	else if (q < vi->curr_queue_pairs)

@@ -1908,7 +1912,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		struct virtqueue *vq = vi->sq[i].vq;
 		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (!is_xdp_queue(vi, i))
+			if (!is_xdp_raw_buffer_queue(vi, i))
 				dev_kfree_skb(buf);
 			else
 				put_page(virt_to_head_page(buf));

[net PATCH v4 2/6] virtio_net: wrap rtnl_lock in test for calling with lock already held

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 00:00:10

For XDP use case and to allow ethtool reset tests it is useful to be
able to use reset paths from contexts where rtnl lock is already
held.

This requries updating virtnet_set_queues and free_receive_bufs the
two places where rtnl_lock is taken in virtio_net. To do this we
use the following pattern,

	_foo(...) { do stuff }
	foo(...) { rtnl_lock(); _foo(...); rtnl_unlock()};

And then in locations that were previously locked,

	if (is_rtnl_locked()) _foo(); else foo();

this allows us to use freeze()/restore() flow from both contexts.

Signed-off-by: John Fastabend <redacted>
---
 drivers/net/virtio_net.c |   41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d97bb71..bc3b1f8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -1331,7 +1331,7 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
 	rtnl_unlock();
 }
 
-static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
+static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 {
 	struct scatterlist sg;
 	struct net_device *dev = vi->dev;

@@ -1357,6 +1357,16 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	return 0;
 }
 
+static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
+{
+	int err;
+
+	rtnl_lock();
+	err = _virtnet_set_queues(vi, queue_pairs);
+	rtnl_unlock();
+	return err;
+}
+
 static int virtnet_close(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);

@@ -1609,7 +1619,7 @@ static int virtnet_set_channels(struct net_device *dev,
 		return -EINVAL;
 
 	get_online_cpus();
-	err = virtnet_set_queues(vi, queue_pairs);
+	err = _virtnet_set_queues(vi, queue_pairs);
 	if (!err) {
 		netif_set_real_num_tx_queues(dev, queue_pairs);
 		netif_set_real_num_rx_queues(dev, queue_pairs);

@@ -1736,7 +1746,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 		return -ENOMEM;
 	}
 
-	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
+	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
 	if (err) {
 		dev_warn(&dev->dev, "XDP Device queue allocation failure.\n");
 		return err;

@@ -1745,7 +1755,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	if (prog) {
 		prog = bpf_prog_add(prog, vi->max_queue_pairs - 1);
 		if (IS_ERR(prog)) {
-			virtnet_set_queues(vi, curr_qp);
+			_virtnet_set_queues(vi, curr_qp);
 			return PTR_ERR(prog);
 		}
 	}

@@ -1864,12 +1874,11 @@ static void virtnet_free_queues(struct virtnet_info *vi)
 	kfree(vi->sq);
 }
 
-static void free_receive_bufs(struct virtnet_info *vi)
+static void _free_receive_bufs(struct virtnet_info *vi)
 {
 	struct bpf_prog *old_prog;
 	int i;
 
-	rtnl_lock();
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		while (vi->rq[i].pages)
 			__free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);

@@ -1879,6 +1888,12 @@ static void free_receive_bufs(struct virtnet_info *vi)
 		if (old_prog)
 			bpf_prog_put(old_prog);
 	}
+}
+
+static void free_receive_bufs(struct virtnet_info *vi)
+{
+	rtnl_lock();
+	_free_receive_bufs(vi);
 	rtnl_unlock();
 }

@@ -2317,9 +2332,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 		goto free_unregister_netdev;
 	}
 
-	rtnl_lock();
 	virtnet_set_queues(vi, vi->curr_queue_pairs);
-	rtnl_unlock();
 
 	/* Assume link up if device can't report link status,
 	   otherwise get link status from config. */

@@ -2358,7 +2371,10 @@ static void remove_vq_common(struct virtnet_info *vi)
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	free_receive_bufs(vi);
+	if (rtnl_is_locked())
+		_free_receive_bufs(vi);
+	else
+		free_receive_bufs(vi);
 
 	free_receive_page_frags(vi);

@@ -2428,9 +2444,10 @@ static int virtnet_restore(struct virtio_device *vdev)
 
 	netif_device_attach(vi->dev);
 
-	rtnl_lock();
-	virtnet_set_queues(vi, vi->curr_queue_pairs);
-	rtnl_unlock();
+	if (rtnl_is_locked())
+		_virtnet_set_queues(vi, vi->curr_queue_pairs);
+	else
+		virtnet_set_queues(vi, vi->curr_queue_pairs);
 
 	err = virtnet_cpu_notif_add(vi);
 	if (err)

[net PATCH v4 3/6] virtio_net: factor out xdp handler for readability

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 00:00:43

At this point the do_xdp_prog is mostly if/else branches handling
the different modes of virtio_net. So remove it and handle running
the program in the per mode handlers.

Signed-off-by: John Fastabend <redacted>
---
 drivers/net/virtio_net.c |   75 +++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 48 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bc3b1f8..7dda206 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -388,49 +388,6 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,
 	virtqueue_kick(sq->vq);
 }
 
-static u32 do_xdp_prog(struct virtnet_info *vi,
-		       struct receive_queue *rq,
-		       struct bpf_prog *xdp_prog,
-		       void *data, int len)
-{
-	int hdr_padded_len;
-	struct xdp_buff xdp;
-	void *buf;
-	unsigned int qp;
-	u32 act;
-
-	if (vi->mergeable_rx_bufs) {
-		hdr_padded_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
-		xdp.data = data + hdr_padded_len;
-		xdp.data_end = xdp.data + (len - vi->hdr_len);
-		buf = data;
-	} else { /* small buffers */
-		struct sk_buff *skb = data;
-
-		xdp.data = skb->data;
-		xdp.data_end = xdp.data + len;
-		buf = skb->data;
-	}
-
-	act = bpf_prog_run_xdp(xdp_prog, &xdp);
-	switch (act) {
-	case XDP_PASS:
-		return XDP_PASS;
-	case XDP_TX:
-		qp = vi->curr_queue_pairs -
-			vi->xdp_queue_pairs +
-			smp_processor_id();
-		xdp.data = buf;
-		virtnet_xdp_xmit(vi, rq, &vi->sq[qp], &xdp, data);
-		return XDP_TX;
-	default:
-		bpf_warn_invalid_xdp_action(act);
-	case XDP_ABORTED:
-	case XDP_DROP:
-		return XDP_DROP;
-	}
-}
-
 static struct sk_buff *receive_small(struct net_device *dev,
 				     struct virtnet_info *vi,
 				     struct receive_queue *rq,

@@ -446,19 +403,30 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	xdp_prog = rcu_dereference(rq->xdp_prog);
 	if (xdp_prog) {
 		struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
+		struct xdp_buff xdp;
+		unsigned int qp;
 		u32 act;
 
 		if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
 			goto err_xdp;
-		act = do_xdp_prog(vi, rq, xdp_prog, skb, len);
+
+		xdp.data = skb->data;
+		xdp.data_end = xdp.data + len;
+		act = bpf_prog_run_xdp(xdp_prog, &xdp);
 		switch (act) {
 		case XDP_PASS:
 			break;
 		case XDP_TX:
+			qp = vi->curr_queue_pairs -
+				vi->xdp_queue_pairs +
+				smp_processor_id();
+			virtnet_xdp_xmit(vi, rq, &vi->sq[qp], &xdp, skb);
 			rcu_read_unlock();
 			goto xdp_xmit;
-		case XDP_DROP:
 		default:
+			bpf_warn_invalid_xdp_action(act);
+		case XDP_ABORTED:
+		case XDP_DROP:
 			goto err_xdp;
 		}
 	}

@@ -576,6 +544,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	xdp_prog = rcu_dereference(rq->xdp_prog);
 	if (xdp_prog) {
 		struct page *xdp_page;
+		struct xdp_buff xdp;
+		unsigned int qp;
+		void *data;
 		u32 act;
 
 		/* This happens when rx buffer size is underestimated */

@@ -598,8 +569,10 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		if (unlikely(hdr->hdr.gso_type))
 			goto err_xdp;
 
-		act = do_xdp_prog(vi, rq, xdp_prog,
-				  page_address(xdp_page) + offset, len);
+		data = page_address(xdp_page) + offset;
+		xdp.data = data + vi->hdr_len;
+		xdp.data_end = xdp.data + (len - vi->hdr_len);
+		act = bpf_prog_run_xdp(xdp_prog, &xdp);
 		switch (act) {
 		case XDP_PASS:
 			/* We can only create skb based on xdp_page. */

@@ -613,13 +586,19 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			}
 			break;
 		case XDP_TX:
+			qp = vi->curr_queue_pairs -
+				vi->xdp_queue_pairs +
+				smp_processor_id();
+			virtnet_xdp_xmit(vi, rq, &vi->sq[qp], &xdp, data);
 			ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
 			if (unlikely(xdp_page != page))
 				goto err_xdp;
 			rcu_read_unlock();
 			goto xdp_xmit;
-		case XDP_DROP:
 		default:
+			bpf_warn_invalid_xdp_action(act);
+		case XDP_ABORTED:
+		case XDP_DROP:
 			if (unlikely(xdp_page != page))
 				__free_pages(xdp_page, 0);
 			ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);

[net PATCH v4 4/6] virtio_net: remove duplicate queue pair binding in XDP

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 00:01:14

Factor out qp assignment.

Signed-off-by: John Fastabend <redacted>
---
 drivers/net/virtio_net.c |   18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7dda206..ac853b9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -332,15 +332,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 
 static void virtnet_xdp_xmit(struct virtnet_info *vi,
 			     struct receive_queue *rq,
-			     struct send_queue *sq,
 			     struct xdp_buff *xdp,
 			     void *data)
 {
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	unsigned int num_sg, len;
+	struct send_queue *sq;
+	unsigned int qp;
 	void *xdp_sent;
 	int err;
 
+	qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
+	sq = &vi->sq[qp];
+
 	/* Free up any pending old buffers before queueing new ones. */
 	while ((xdp_sent = virtqueue_get_buf(sq->vq, &len)) != NULL) {
 		if (vi->mergeable_rx_bufs) {

@@ -404,7 +408,6 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	if (xdp_prog) {
 		struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
 		struct xdp_buff xdp;
-		unsigned int qp;
 		u32 act;
 
 		if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))

@@ -417,10 +420,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 		case XDP_PASS:
 			break;
 		case XDP_TX:
-			qp = vi->curr_queue_pairs -
-				vi->xdp_queue_pairs +
-				smp_processor_id();
-			virtnet_xdp_xmit(vi, rq, &vi->sq[qp], &xdp, skb);
+			virtnet_xdp_xmit(vi, rq, &xdp, skb);
 			rcu_read_unlock();
 			goto xdp_xmit;
 		default:

@@ -545,7 +545,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	if (xdp_prog) {
 		struct page *xdp_page;
 		struct xdp_buff xdp;
-		unsigned int qp;
 		void *data;
 		u32 act;

@@ -586,10 +585,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			}
 			break;
 		case XDP_TX:
-			qp = vi->curr_queue_pairs -
-				vi->xdp_queue_pairs +
-				smp_processor_id();
-			virtnet_xdp_xmit(vi, rq, &vi->sq[qp], &xdp, data);
+			virtnet_xdp_xmit(vi, rq, &xdp, data);
 			ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
 			if (unlikely(xdp_page != page))
 				goto err_xdp;

[net PATCH v4 5/6] virtio: add pci_down/pci_up configuration

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 00:01:39

In virtio_net we need to do a full reset of the device to support
queue reconfiguration and also we can trigger this via ethtool
commands. So instead of open coding this in net driver push this
into generic code in virtio. This also avoid exporting a handful
of internal virtio routines.

Signed-off-by: John Fastabend <redacted>
---
 drivers/virtio/virtio.c         |   14 ++++++++++++--
 drivers/virtio/virtio_balloon.c |    4 ----
 drivers/virtio/virtio_input.c   |    4 ----
 include/linux/virtio.h          |    5 +----
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 7062bb0..681fcfb 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c

@@ -339,7 +339,6 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
-#ifdef CONFIG_PM_SLEEP
 int virtio_device_freeze(struct virtio_device *dev)
 {
 	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);

@@ -400,7 +399,18 @@ int virtio_device_restore(struct virtio_device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(virtio_device_restore);
-#endif
+
+int virtio_device_reset(struct virtio_device *dev)
+{
+	int err;
+
+	err = virtio_device_freeze(dev);
+	if (err)
+		return err;
+
+	return virtio_device_restore(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_device_reset);
 
 static int virtio_init(void)
 {

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 181793f..4ca6220 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c

@@ -620,7 +620,6 @@ static void virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int virtballoon_freeze(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb = vdev->priv;

@@ -649,7 +648,6 @@ static int virtballoon_restore(struct virtio_device *vdev)
 	update_balloon_size(vb);
 	return 0;
 }
-#endif
 
 static unsigned int features[] = {
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,

@@ -666,10 +664,8 @@ static int virtballoon_restore(struct virtio_device *vdev)
 	.probe =	virtballoon_probe,
 	.remove =	virtballoon_remove,
 	.config_changed = virtballoon_changed,
-#ifdef CONFIG_PM_SLEEP
 	.freeze	=	virtballoon_freeze,
 	.restore =	virtballoon_restore,
-#endif
 };
 
 module_virtio_driver(virtio_balloon_driver);

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index 350a2a5..d3517e2 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c

@@ -328,7 +328,6 @@ static void virtinput_remove(struct virtio_device *vdev)
 	kfree(vi);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int virtinput_freeze(struct virtio_device *vdev)
 {
 	struct virtio_input *vi = vdev->priv;

@@ -356,7 +355,6 @@ static int virtinput_restore(struct virtio_device *vdev)
 	virtinput_fill_evt(vi);
 	return 0;
 }
-#endif
 
 static unsigned int features[] = {
 	/* none */

@@ -374,10 +372,8 @@ static int virtinput_restore(struct virtio_device *vdev)
 	.id_table            = id_table,
 	.probe               = virtinput_probe,
 	.remove              = virtinput_remove,
-#ifdef CONFIG_PM_SLEEP
 	.freeze	             = virtinput_freeze,
 	.restore             = virtinput_restore,
-#endif
 };
 
 module_virtio_driver(virtio_input_driver);

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d5eb547..ff69f9a 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h

@@ -138,10 +138,9 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 void virtio_break_device(struct virtio_device *dev);
 
 void virtio_config_changed(struct virtio_device *dev);
-#ifdef CONFIG_PM_SLEEP
 int virtio_device_freeze(struct virtio_device *dev);
 int virtio_device_restore(struct virtio_device *dev);
-#endif
+int virtio_device_reset(struct virtio_device *dev);
 
 /**
  * virtio_driver - operations for a virtio I/O driver

@@ -167,10 +166,8 @@ struct virtio_driver {
 	void (*scan)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
 	void (*config_changed)(struct virtio_device *dev);
-#ifdef CONFIG_PM
 	int (*freeze)(struct virtio_device *dev);
 	int (*restore)(struct virtio_device *dev);
-#endif
 };
 
 static inline struct virtio_driver *drv_to_virtio(struct device_driver *drv)

[net PATCH v4 6/6] virtio_net: XDP support for adjust_head

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 00:02:03

Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.

In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.

At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.

Signed-off-by: John Fastabend <redacted>
---
 drivers/net/virtio_net.c |  110 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ac853b9..2cc59db 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -41,6 +41,9 @@
 #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
 #define GOOD_COPY_LEN	128
 
+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */
+#define VIRTIO_XDP_HEADROOM 256
+
 /* RX packet size EWMA. The average packet size is used to determine the packet
  * buffer size when refilling RX rings. As the entire RX ring may be refilled
  * at once, the weight is chosen so that the EWMA will be insensitive to short-

@@ -359,6 +362,7 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,
 	}
 
 	if (vi->mergeable_rx_bufs) {
+		xdp->data -= sizeof(struct virtio_net_hdr_mrg_rxbuf);
 		/* Zero header and leave csum up to XDP layers */
 		hdr = xdp->data;
 		memset(hdr, 0, vi->hdr_len);

@@ -375,7 +379,9 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,
 		num_sg = 2;
 		sg_init_table(sq->sg, 2);
 		sg_set_buf(sq->sg, hdr, vi->hdr_len);
-		skb_to_sgvec(skb, sq->sg + 1, 0, skb->len);
+		skb_to_sgvec(skb, sq->sg + 1,
+			     xdp->data - xdp->data_hard_start,
+			     xdp->data_end - xdp->data);
 	}
 	err = virtqueue_add_outbuf(sq->vq, sq->sg, num_sg,
 				   data, GFP_ATOMIC);

@@ -401,7 +407,6 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	struct bpf_prog *xdp_prog;
 
 	len -= vi->hdr_len;
-	skb_trim(skb, len);
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);

@@ -413,11 +418,15 @@ static struct sk_buff *receive_small(struct net_device *dev,
 		if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
 			goto err_xdp;
 
-		xdp.data = skb->data;
+		xdp.data_hard_start = skb->data;
+		xdp.data = skb->data + VIRTIO_XDP_HEADROOM;
 		xdp.data_end = xdp.data + len;
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
 		switch (act) {
 		case XDP_PASS:
+			/* Recalculate length in case bpf program changed it */
+			__skb_pull(skb, xdp.data - xdp.data_hard_start);
+			len = xdp.data_end - xdp.data;
 			break;
 		case XDP_TX:
 			virtnet_xdp_xmit(vi, rq, &xdp, skb);

@@ -432,6 +441,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	}
 	rcu_read_unlock();
 
+	skb_trim(skb, len);
 	return skb;
 
 err_xdp:

@@ -480,7 +490,7 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
 				       unsigned int *len)
 {
 	struct page *page = alloc_page(GFP_ATOMIC);
-	unsigned int page_off = 0;
+	unsigned int page_off = VIRTIO_XDP_HEADROOM;
 
 	if (!page)
 		return NULL;

@@ -516,7 +526,8 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
 		put_page(p);
 	}
 
-	*len = page_off;
+	/* Headroom does not contribute to packet length */
+	*len = page_off - VIRTIO_XDP_HEADROOM;
 	return page;
 err_buf:
 	__free_pages(page, 0);

@@ -555,7 +566,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 						      page, offset, &len);
 			if (!xdp_page)
 				goto err_xdp;
-			offset = 0;
+			offset = VIRTIO_XDP_HEADROOM;
 		} else {
 			xdp_page = page;
 		}

@@ -568,18 +579,29 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		if (unlikely(hdr->hdr.gso_type))
 			goto err_xdp;
 
+		/* Allow consuming headroom but reserve enough space to push
+		 * the descriptor on if we get an XDP_TX return code.
+		 */
 		data = page_address(xdp_page) + offset;
+		xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
 		xdp.data = data + vi->hdr_len;
 		xdp.data_end = xdp.data + (len - vi->hdr_len);
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
 		switch (act) {
 		case XDP_PASS:
+			/* recalculate offset to account for any header
+			 * adjustments. Note other cases do not build an
+			 * skb and avoid using offset
+			 */
+			offset = xdp.data -
+					page_address(xdp_page) - vi->hdr_len;
+
 			/* We can only create skb based on xdp_page. */
 			if (unlikely(xdp_page != page)) {
 				rcu_read_unlock();
 				put_page(page);
 				head_skb = page_to_skb(vi, rq, xdp_page,
-						       0, len, PAGE_SIZE);
+						       offset, len, PAGE_SIZE);
 				ewma_pkt_len_add(&rq->mrg_avg_pkt_len, len);
 				return head_skb;
 			}

@@ -744,23 +766,30 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 	dev_kfree_skb(skb);
 }
 
+static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
+{
+	return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
+}
+
 static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 			     gfp_t gfp)
 {
+	int headroom = GOOD_PACKET_LEN + virtnet_get_headroom(vi);
+	unsigned int xdp_headroom = virtnet_get_headroom(vi);
 	struct sk_buff *skb;
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
 	int err;
 
-	skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp);
+	skb = __netdev_alloc_skb_ip_align(vi->dev, headroom, gfp);
 	if (unlikely(!skb))
 		return -ENOMEM;
 
-	skb_put(skb, GOOD_PACKET_LEN);
+	skb_put(skb, headroom);
 
 	hdr = skb_vnet_hdr(skb);
 	sg_init_table(rq->sg, 2);
 	sg_set_buf(rq->sg, hdr, vi->hdr_len);
-	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
+	skb_to_sgvec(skb, rq->sg + 1, xdp_headroom, skb->len - xdp_headroom);
 
 	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)

@@ -828,24 +857,27 @@ static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)
 	return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
 }
 
-static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
+static int add_recvbuf_mergeable(struct virtnet_info *vi,
+				 struct receive_queue *rq, gfp_t gfp)
 {
 	struct page_frag *alloc_frag = &rq->alloc_frag;
+	unsigned int headroom = virtnet_get_headroom(vi);
 	char *buf;
 	unsigned long ctx;
 	int err;
 	unsigned int len, hole;
 
 	len = get_mergeable_buf_len(&rq->mrg_avg_pkt_len);
-	if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
+	if (unlikely(!skb_page_frag_refill(len + headroom, alloc_frag, gfp)))
 		return -ENOMEM;
 
 	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
+	buf += headroom; /* advance address leaving hole at front of pkt */
 	ctx = mergeable_buf_to_ctx(buf, len);
 	get_page(alloc_frag->page);
-	alloc_frag->offset += len;
+	alloc_frag->offset += len + headroom;
 	hole = alloc_frag->size - alloc_frag->offset;
-	if (hole < len) {
+	if (hole < len + headroom) {
 		/* To avoid internal fragmentation, if there is very likely not
 		 * enough space for another buffer, add the remaining space to
 		 * the current buffer. This extra space is not included in

@@ -879,7 +911,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 	gfp |= __GFP_COLD;
 	do {
 		if (vi->mergeable_rx_bufs)
-			err = add_recvbuf_mergeable(rq, gfp);
+			err = add_recvbuf_mergeable(vi, rq, gfp);
 		else if (vi->big_packets)
 			err = add_recvbuf_big(vi, rq, gfp);
 		else

@@ -1689,7 +1721,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct bpf_prog *old_prog;
-	u16 xdp_qp = 0, curr_qp;
+	u16 oxdp_qp, xdp_qp = 0, curr_qp;
 	int i, err;
 
 	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||

@@ -1721,21 +1753,32 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 		return -ENOMEM;
 	}
 
+	if (prog) {
+		prog = bpf_prog_add(prog, vi->max_queue_pairs - 1);
+		if (IS_ERR(prog))
+			return PTR_ERR(prog);
+	}
+
 	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
 	if (err) {
 		dev_warn(&dev->dev, "XDP Device queue allocation failure.\n");
-		return err;
+		goto virtio_queue_err;
 	}
 
-	if (prog) {
-		prog = bpf_prog_add(prog, vi->max_queue_pairs - 1);
-		if (IS_ERR(prog)) {
-			_virtnet_set_queues(vi, curr_qp);
-			return PTR_ERR(prog);
-		}
+	oxdp_qp = vi->xdp_queue_pairs;
+
+	/* Changing the headroom in buffers is a disruptive operation because
+	 * existing buffers must be flushed and reallocated. This will happen
+	 * when a xdp program is initially added or xdp is disabled by removing
+	 * the xdp program resulting in number of XDP queues changing.
+	 */
+	if (vi->xdp_queue_pairs != xdp_qp) {
+		vi->xdp_queue_pairs = xdp_qp;
+		err = virtio_device_reset(vi->vdev);
+		if (err)
+			goto virtio_reset_err;
 	}
 
-	vi->xdp_queue_pairs = xdp_qp;
 	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {

@@ -1746,6 +1789,21 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	}
 
 	return 0;
+
+virtio_reset_err:
+	/* On reset error do our best to unwind XDP changes inflight and return
+	 * error up to user space for resolution. The underlying PCI reset hung
+	 * on us so not much we can do here.
+	 */
+	dev_warn(&dev->dev, "XDP reset failure and queues unstable\n");
+	vi->xdp_queue_pairs = oxdp_qp;
+virtio_queue_err:
+	/* On queue set error we can unwind bpf ref count and user space can
+	 * retry this is most likely an allocation failure.
+	 */
+	if (prog)
+		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
+	return err;
 }
 
 static bool virtnet_xdp_query(struct net_device *dev)

@@ -2373,7 +2431,6 @@ static void virtnet_remove(struct virtio_device *vdev)
 	free_netdev(vi->dev);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int virtnet_freeze(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;

@@ -2430,7 +2487,6 @@ static int virtnet_restore(struct virtio_device *vdev)
 
 	return 0;
 }
-#endif
 
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },

@@ -2470,10 +2526,8 @@ static int virtnet_restore(struct virtio_device *vdev)
 	.probe =	virtnet_probe,
 	.remove =	virtnet_remove,
 	.config_changed = virtnet_config_changed,
-#ifdef CONFIG_PM_SLEEP
 	.freeze =	virtnet_freeze,
 	.restore =	virtnet_restore,
-#endif
 };
 
 static __init int virtio_net_driver_init(void)

Re: [net PATCH v4 5/6] virtio: add pci_down/pci_up configuration

From: Jason Wang <hidden>
Date: 2017-01-16 03:57:33


On 2017年01月16日 08:01, John Fastabend wrote:

In virtio_net we need to do a full reset of the device to support
queue reconfiguration and also we can trigger this via ethtool
commands. So instead of open coding this in net driver push this
into generic code in virtio. This also avoid exporting a handful
of internal virtio routines.

Looks like this is not a pci specific stuffs. And there's some driver 
left (e.g scsi and block).

In fact, I'm not sure touching other drivers is really needed. Maybe we 
can just:
- move virtio_device_freeze(), virtio_device_restore() and 
.freeze/.restore in virtio_driver out of CONFIG_PM_SLEEP
- move virtnet_freeze() and virtnet_restore() out of CONFIG_PM_SLEEP
- introduce virtio_net_reset() and call 
virtio_device_freeze()/virtio_device_restore() there

Another possible issue for sleep/hibernation is xdp_prog were not 
restored, if this is not XDP intended, we'd better fix this.

Thanks

[...]

Re: [net PATCH v4 5/6] virtio: add pci_down/pci_up configuration

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-16 04:04:47

On 17-01-15 07:57 PM, Jason Wang wrote:


On 2017年01月16日 08:01, John Fastabend wrote:

quoted

In virtio_net we need to do a full reset of the device to support
queue reconfiguration and also we can trigger this via ethtool
commands. So instead of open coding this in net driver push this
into generic code in virtio. This also avoid exporting a handful
of internal virtio routines.

Looks like this is not a pci specific stuffs. And there's some driver left (e.g
scsi and block).

In fact, I'm not sure touching other drivers is really needed. Maybe we can just:
- move virtio_device_freeze(), virtio_device_restore() and .freeze/.restore in
virtio_driver out of CONFIG_PM_SLEEP
- move virtnet_freeze() and virtnet_restore() out of CONFIG_PM_SLEEP
- introduce virtio_net_reset() and call
virtio_device_freeze()/virtio_device_restore() there

Michael, what do you think? I was following your suggestion on writing a
generic virtio reset here. I don't have a preference either way.

Another possible issue for sleep/hibernation is xdp_prog were not restored, if
this is not XDP intended, we'd better fix this.

Yep this bug exists even without this series. I'll add it to my list of things
to fix. Along with small packet max_mtu.

Thanks

[...]

Re: [net PATCH v4 6/6] virtio_net: XDP support for adjust_head

From: Jason Wang <hidden>
Date: 2017-01-16 05:48:26


On 2017年01月16日 08:01, John Fastabend wrote:

Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.

In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.

At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.

Signed-off-by: John Fastabend <redacted>
---
  drivers/net/virtio_net.c |  110 ++++++++++++++++++++++++++++++++++------------
  1 file changed, 82 insertions(+), 28 deletions(-)

[...]

quoted hunk

-	vi->xdp_queue_pairs = xdp_qp;
  	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
  
  	for (i = 0; i < vi->max_queue_pairs; i++) {

@@ -1746,6 +1789,21 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
  	}
  
  	return 0;
+
+virtio_reset_err:
+	/* On reset error do our best to unwind XDP changes inflight and return
+	 * error up to user space for resolution. The underlying PCI reset hung
+	 * on us so not much we can do here.

It should work with other transport, so let's remove "PCI" here.

quoted hunk

+	 */
+	dev_warn(&dev->dev, "XDP reset failure and queues unstable\n");
+	vi->xdp_queue_pairs = oxdp_qp;
+virtio_queue_err:
+	/* On queue set error we can unwind bpf ref count and user space can
+	 * retry this is most likely an allocation failure.
+	 */
+	if (prog)
+		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
+	return err;
  }
  
  static bool virtnet_xdp_query(struct net_device *dev)

@@ -2373,7 +2431,6 @@ static void virtnet_remove(struct virtio_device *vdev)
  	free_netdev(vi->dev);
  }
  
-#ifdef CONFIG_PM_SLEEP
  static int virtnet_freeze(struct virtio_device *vdev)
  {
  	struct virtnet_info *vi = vdev->priv;
@@ -2430,7 +2487,6 @@ static int virtnet_restore(struct virtio_device *vdev)
  
  	return 0;
  }
-#endif

If you do want to use virtio_device_reset(), it's better to squash this 
into patch 5/6.

Other looks good.

Thanks

Re: [net PATCH v4 0/6] virtio_net XDP fixes and adjust_header support

From: Jason Wang <hidden>
Date: 2017-01-16 05:51:39


On 2017年01月16日 07:59, John Fastabend wrote:

This has a fix to handle small buffer free logic correctly and then
also adds adjust head support.

I pushed adjust head at net (even though its rc3) to avoid having
to push another exception case into virtio_net to catch if the
program uses adjust_head and then block it. If there are any strong
objections to this we can push it at net-next and use a patch from
Jakub to add the exception handling but then user space has to deal
with it either via try/fail logic or via kernel version checks. Granted
we already have some cases that need to be configured to enable XDP
but I don't see any reason to have yet another one when we can fix it
now vs delaying a kernel version.


v2: fix spelling error, convert unsigned -> unsigned int
v3: v2 git crashed during send so retrying sorry for the noise
v4: changed layout of rtnl_lock fixes (Stephen)
     moved reset logic into virtio core with new patch (MST)
     fixed up linearize and some code cleanup (Jason)

     Otherwise did some generic code cleanup so might be a bit
     cleaner this time at least that is the hope.

Thanks everyone for the v3 review.

Thanks, looks good to me overall, just few nits.

Re: [net PATCH v4 2/6] virtio_net: wrap rtnl_lock in test for calling with lock already held

From: David Miller <davem@davemloft.net>
Date: 2017-01-17 17:06:56

From: John Fastabend <john.fastabend@gmail.com>
Date: Sun, 15 Jan 2017 15:59:47 -0800

quoted hunk

@@ -2358,7 +2371,10 @@ static void remove_vq_common(struct virtnet_info *vi)
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	free_receive_bufs(vi);
+	if (rtnl_is_locked())
+		_free_receive_bufs(vi);
+	else
+		free_receive_bufs(vi);
 
 	free_receive_page_frags(vi);
 

This doesn't work.  rtnl_is_locked() doesn't tell if _you_ own the mutex, it
just says that someone does.

So if we now execute this code without taking the RTNL lock just because some
other thread of control holds it, we introduce a race.

Re: [net PATCH v4 5/6] virtio: add pci_down/pci_up configuration

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-17 18:45:41

On 17-01-15 08:04 PM, John Fastabend wrote:

On 17-01-15 07:57 PM, Jason Wang wrote:

quoted


On 2017年01月16日 08:01, John Fastabend wrote:

quoted

In virtio_net we need to do a full reset of the device to support
queue reconfiguration and also we can trigger this via ethtool
commands. So instead of open coding this in net driver push this
into generic code in virtio. This also avoid exporting a handful
of internal virtio routines.

Looks like this is not a pci specific stuffs. And there's some driver left (e.g
scsi and block).

In fact, I'm not sure touching other drivers is really needed. Maybe we can just:
- move virtio_device_freeze(), virtio_device_restore() and .freeze/.restore in
virtio_driver out of CONFIG_PM_SLEEP
- move virtnet_freeze() and virtnet_restore() out of CONFIG_PM_SLEEP
- introduce virtio_net_reset() and call
virtio_device_freeze()/virtio_device_restore() there

Michael, what do you think? I was following your suggestion on writing a
generic virtio reset here. I don't have a preference either way.

Actually if I roll it into virtio_net per above suggestion I can probably get
the locking correct. As DaveM points out its complete junk at the moment around
the rtnl_try_lock() logic. Ugh.

I'll spin a v5...

quoted

Another possible issue for sleep/hibernation is xdp_prog were not restored, if
this is not XDP intended, we'd better fix this.

Yep this bug exists even without this series. I'll add it to my list of things
to fix. Along with small packet max_mtu.

quoted

Thanks

[...]

Re: [net PATCH v4 2/6] virtio_net: wrap rtnl_lock in test for calling with lock already held

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-01-17 19:03:34

On 17-01-17 08:57 AM, David Miller wrote:

From: John Fastabend <john.fastabend@gmail.com>
Date: Sun, 15 Jan 2017 15:59:47 -0800

quoted

@@ -2358,7 +2371,10 @@ static void remove_vq_common(struct virtnet_info *vi)
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	free_receive_bufs(vi);
+	if (rtnl_is_locked())
+		_free_receive_bufs(vi);
+	else
+		free_receive_bufs(vi);
 
 	free_receive_page_frags(vi);
 

This doesn't work.  rtnl_is_locked() doesn't tell if _you_ own the mutex, it
just says that someone does.

So if we now execute this code without taking the RTNL lock just because some
other thread of control holds it, we introduce a race.

yeah this bit is junk. dang. Trying to get this locking right without duplicate
code or pushing around lock_me variables is getting tricky.

.John

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help