[PATCH net-next v9 0/2] xen networking: add XDP support to xen-netfront

[PATCH net-next v9 0/2] xen networking: add XDP support to xen-netfront

[Denis Kirjanov]

This series adds XDP support to xen-nefront driver.
The second patch enables extra space for XDP processing.

v9:
- assign an xdp program before switching to Reconfiguring
- minor cleanups
- address checkpatch issues

v8:
- add PAGE_POOL config dependency
- keep the state of XDP processing in netfront_xdp_enabled
- fixed allocator type in xdp_rxq_info_reg_mem_model()
- minor cleanups in xen-netback

v7:
- use page_pool_dev_alloc_pages() on page allocation
- remove the leftover break statement from netback_changed

v6:
- added the missing SOB line
- fixed subject

v5:
- split netfront/netback changes
- added a sync point between backend/frontend on switching to XDP
- added pagepool API

v4:
- added verbose patch descriprion
- don't expose the XDP headroom offset to the domU guest
- add a modparam to netback to toggle XDP offset
- don't process jumbo frames for now

v3:
- added XDP_TX support (tested with xdping echoserver)
- added XDP_REDIRECT support (tested with modified xdp_redirect_kern)
- moved xdp negotiation to xen-netback

v2:
- avoid data copying while passing to XDP
- tell xen-netback that we need the headroom space

Denis Kirjanov (2):
  xen networking: add basic XDP support for xen-netfront
  xen networking: add XDP offset adjustment to xen-netback

 drivers/net/Kconfig               |   1 +
 drivers/net/xen-netback/common.h  |   2 +
 drivers/net/xen-netback/netback.c |   7 +
 drivers/net/xen-netback/rx.c      |   7 +-
 drivers/net/xen-netback/xenbus.c  |  28 ++++
 drivers/net/xen-netfront.c        | 317 +++++++++++++++++++++++++++++++++++++-
 6 files changed, 355 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Denis Kirjanov]

The patch adds a basic XDP processing to xen-netfront driver.

We ran an XDP program for an RX response received from netback
driver. Also we request xen-netback to adjust data offset for
bpf_xdp_adjust_head() header space for custom headers.

synchronization between frontend and backend parts is done
by using xenbus state switching:
Reconfiguring -> Reconfigured- > Connected

UDP packets drop rate using xdp program is around 310 kpps
using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.

Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
---
 drivers/net/Kconfig        |   1 +
 drivers/net/xen-netfront.c | 317 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 312 insertions(+), 6 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 25a8f93..45918ce 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -479,6 +479,7 @@ config XEN_NETDEV_FRONTEND
 	tristate "Xen network device frontend driver"
 	depends on XEN
 	select XEN_XENBUS_FRONTEND
+	select PAGE_POOL
 	default y
 	help
 	  This driver provides support for Xen paravirtual network
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 482c6c8..33544c7 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -44,6 +44,9 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <net/ip.h>
+#include <linux/bpf.h>
+#include <net/page_pool.h>
+#include <linux/bpf_trace.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -102,6 +105,8 @@ struct netfront_queue {
 	char name[QUEUE_NAME_SIZE]; /* DEVNAME-qN */
 	struct netfront_info *info;
 
+	struct bpf_prog __rcu *xdp_prog;
+
 	struct napi_struct napi;
 
 	/* Split event channels support, tx_* == rx_* when using
@@ -144,6 +149,9 @@ struct netfront_queue {
 	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
 	grant_ref_t gref_rx_head;
 	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
+
+	struct page_pool *page_pool;
+	struct xdp_rxq_info xdp_rxq;
 };
 
 struct netfront_info {
@@ -159,6 +167,10 @@ struct netfront_info {
 	struct netfront_stats __percpu *rx_stats;
 	struct netfront_stats __percpu *tx_stats;
 
+	/* XDP state */
+	bool netback_has_xdp_headroom;
+	bool netfront_xdp_enabled;
+
 	atomic_t rx_gso_checksum_fixup;
 };
 
@@ -265,8 +277,8 @@ static struct sk_buff *xennet_alloc_one_rx_buffer(struct netfront_queue *queue)
 	if (unlikely(!skb))
 		return NULL;
 
-	page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
-	if (!page) {
+	page = page_pool_dev_alloc_pages(queue->page_pool);
+	if (unlikely(!page)) {
 		kfree_skb(skb);
 		return NULL;
 	}
@@ -560,6 +572,64 @@ static u16 xennet_select_queue(struct net_device *dev, struct sk_buff *skb,
 	return queue_idx;
 }
 
+static int xennet_xdp_xmit_one(struct net_device *dev, struct xdp_frame *xdpf)
+{
+	struct netfront_info *np = netdev_priv(dev);
+	struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
+	struct netfront_queue *queue = NULL;
+	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned long flags;
+	int notify;
+	struct xen_netif_tx_request *tx;
+
+	queue = &np->queues[smp_processor_id() % num_queues];
+
+	spin_lock_irqsave(&queue->tx_lock, flags);
+
+	tx = xennet_make_first_txreq(queue, NULL,
+				     virt_to_page(xdpf->data),
+				     offset_in_page(xdpf->data),
+				     xdpf->len);
+
+	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&queue->tx, notify);
+	if (notify)
+		notify_remote_via_irq(queue->tx_irq);
+
+	u64_stats_update_begin(&tx_stats->syncp);
+	tx_stats->bytes += xdpf->len;
+	tx_stats->packets++;
+	u64_stats_update_end(&tx_stats->syncp);
+
+	xennet_tx_buf_gc(queue);
+
+	spin_unlock_irqrestore(&queue->tx_lock, flags);
+	return 0;
+}
+
+static int xennet_xdp_xmit(struct net_device *dev, int n,
+			   struct xdp_frame **frames, u32 flags)
+{
+	int drops = 0;
+	int i, err;
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+		return -EINVAL;
+
+	for (i = 0; i < n; i++) {
+		struct xdp_frame *xdpf = frames[i];
+
+		if (!xdpf)
+			continue;
+		err = xennet_xdp_xmit_one(dev, xdpf);
+		if (err) {
+			xdp_return_frame_rx_napi(xdpf);
+			drops++;
+		}
+	}
+
+	return n - drops;
+}
+
 #define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
 
 static netdev_tx_t xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -778,9 +848,56 @@ static int xennet_get_extras(struct netfront_queue *queue,
 	return err;
 }
 
+static u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata,
+			  struct xen_netif_rx_response *rx, struct bpf_prog *prog,
+			  struct xdp_buff *xdp, bool *need_xdp_flush)
+{
+	struct xdp_frame *xdpf;
+	u32 len = rx->status;
+	u32 act = XDP_PASS;
+	int err;
+
+	xdp->data_hard_start = page_address(pdata);
+	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
+	xdp_set_data_meta_invalid(xdp);
+	xdp->data_end = xdp->data + len;
+	xdp->rxq = &queue->xdp_rxq;
+
+	act = bpf_prog_run_xdp(prog, xdp);
+	switch (act) {
+	case XDP_TX:
+		get_page(pdata);
+		xdpf = convert_to_xdp_frame(xdp);
+		err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0);
+		if (unlikely(err < 0))
+			trace_xdp_exception(queue->info->netdev, prog, act);
+		break;
+	case XDP_REDIRECT:
+		get_page(pdata);
+		err = xdp_do_redirect(queue->info->netdev, xdp, prog);
+		*need_xdp_flush = true;
+		if (unlikely(err))
+			trace_xdp_exception(queue->info->netdev, prog, act);
+		break;
+	case XDP_PASS:
+	case XDP_DROP:
+		break;
+
+	case XDP_ABORTED:
+		trace_xdp_exception(queue->info->netdev, prog, act);
+		break;
+
+	default:
+		bpf_warn_invalid_xdp_action(act);
+	}
+
+	return act;
+}
+
 static int xennet_get_responses(struct netfront_queue *queue,
 				struct netfront_rx_info *rinfo, RING_IDX rp,
-				struct sk_buff_head *list)
+				struct sk_buff_head *list,
+				bool *need_xdp_flush)
 {
 	struct xen_netif_rx_response *rx = &rinfo->rx;
 	struct xen_netif_extra_info *extras = rinfo->extras;
@@ -792,6 +909,9 @@ static int xennet_get_responses(struct netfront_queue *queue,
 	int slots = 1;
 	int err = 0;
 	unsigned long ret;
+	struct bpf_prog *xdp_prog;
+	struct xdp_buff xdp;
+	u32 verdict;
 
 	if (rx->flags & XEN_NETRXF_extra_info) {
 		err = xennet_get_extras(queue, extras, rp);
@@ -827,9 +947,24 @@ static int xennet_get_responses(struct netfront_queue *queue,
 
 		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
 
-		__skb_queue_tail(list, skb);
-
+		rcu_read_lock();
+		xdp_prog = rcu_dereference(queue->xdp_prog);
+		if (xdp_prog) {
+			if (!(rx->flags & XEN_NETRXF_more_data)) {
+				/* currently only a single page contains data */
+				verdict = xennet_run_xdp(queue,
+							 skb_frag_page(&skb_shinfo(skb)->frags[0]),
+							 rx, xdp_prog, &xdp, need_xdp_flush);
+				if (verdict != XDP_PASS)
+					err = -EINVAL;
+			} else {
+				/* drop the frame */
+				err = -EINVAL;
+			}
+		}
+		rcu_read_unlock();
 next:
+		__skb_queue_tail(list, skb);
 		if (!(rx->flags & XEN_NETRXF_more_data))
 			break;
 
@@ -997,7 +1132,9 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 	struct sk_buff_head rxq;
 	struct sk_buff_head errq;
 	struct sk_buff_head tmpq;
+	struct bpf_prog *xdp_prog;
 	int err;
+	bool need_xdp_flush = false;
 
 	spin_lock(&queue->rx_lock);
 
@@ -1010,11 +1147,17 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 
 	i = queue->rx.rsp_cons;
 	work_done = 0;
+	rcu_read_lock();
 	while ((i != rp) && (work_done < budget)) {
 		memcpy(rx, RING_GET_RESPONSE(&queue->rx, i), sizeof(*rx));
 		memset(extras, 0, sizeof(rinfo.extras));
 
-		err = xennet_get_responses(queue, &rinfo, rp, &tmpq);
+		xdp_prog = rcu_dereference(queue->xdp_prog);
+		if (xdp_prog)
+			rx->offset = XDP_PACKET_HEADROOM;
+
+		err = xennet_get_responses(queue, &rinfo, rp, &tmpq,
+					   &need_xdp_flush);
 
 		if (unlikely(err)) {
 err:
@@ -1060,6 +1203,9 @@ static int xennet_poll(struct napi_struct *napi, int budget)
 		i = ++queue->rx.rsp_cons;
 		work_done++;
 	}
+	if (need_xdp_flush)
+		xdp_do_flush();
+	rcu_read_unlock();
 
 	__skb_queue_purge(&errq);
 
@@ -1261,6 +1407,98 @@ static void xennet_poll_controller(struct net_device *dev)
 }
 #endif
 
+#define NETBACK_XDP_HEADROOM_DISABLE	0
+#define NETBACK_XDP_HEADROOM_ENABLE	1
+
+static int talk_to_netback_xdp(struct netfront_info *np, int xdp)
+{
+	int err;
+
+	err = xenbus_printf(XBT_NIL, np->xbdev->nodename,
+			    "feature-xdp", "%u", xdp);
+	if (err)
+		pr_debug("Error writing feature-xdp\n");
+
+	return err;
+}
+
+static int xennet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+			  struct netlink_ext_ack *extack)
+{
+	struct netfront_info *np = netdev_priv(dev);
+	struct bpf_prog *old_prog;
+	unsigned int i, err;
+	unsigned long max_mtu = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM;
+
+	if (dev->mtu > max_mtu) {
+		netdev_warn(dev, "XDP requires MTU less than %lu\n", max_mtu);
+		return -EINVAL;
+	}
+
+	if (!np->netback_has_xdp_headroom)
+		return 0;
+
+	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
+
+	err = talk_to_netback_xdp(np, prog ? NETBACK_XDP_HEADROOM_ENABLE :
+				  NETBACK_XDP_HEADROOM_DISABLE);
+	if (err)
+		return err;
+
+	/* avoid race with XDP headroom adjustment */
+	wait_event(module_wq,
+		   xenbus_read_driver_state(np->xbdev->otherend) ==
+		   XenbusStateReconfigured);
+	np->netfront_xdp_enabled = true;
+
+	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
+
+	if (prog)
+		bpf_prog_add(prog, dev->real_num_tx_queues);
+
+	for (i = 0; i < dev->real_num_tx_queues; ++i)
+		rcu_assign_pointer(np->queues[i].xdp_prog, prog);
+
+	if (old_prog)
+		for (i = 0; i < dev->real_num_tx_queues; ++i)
+			bpf_prog_put(old_prog);
+
+	xenbus_switch_state(np->xbdev, XenbusStateConnected);
+
+	return 0;
+}
+
+static u32 xennet_xdp_query(struct net_device *dev)
+{
+	struct netfront_info *np = netdev_priv(dev);
+	unsigned int num_queues = dev->real_num_tx_queues;
+	unsigned int i;
+	struct netfront_queue *queue;
+	const struct bpf_prog *xdp_prog;
+
+	for (i = 0; i < num_queues; ++i) {
+		queue = &np->queues[i];
+		xdp_prog = rtnl_dereference(queue->xdp_prog);
+		if (xdp_prog)
+			return xdp_prog->aux->id;
+	}
+
+	return 0;
+}
+
+static int xennet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
+{
+	switch (xdp->command) {
+	case XDP_SETUP_PROG:
+		return xennet_xdp_set(dev, xdp->prog, xdp->extack);
+	case XDP_QUERY_PROG:
+		xdp->prog_id = xennet_xdp_query(dev);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct net_device_ops xennet_netdev_ops = {
 	.ndo_open            = xennet_open,
 	.ndo_stop            = xennet_close,
@@ -1272,6 +1510,8 @@ static void xennet_poll_controller(struct net_device *dev)
 	.ndo_fix_features    = xennet_fix_features,
 	.ndo_set_features    = xennet_set_features,
 	.ndo_select_queue    = xennet_select_queue,
+	.ndo_bpf            = xennet_xdp,
+	.ndo_xdp_xmit	    = xennet_xdp_xmit,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = xennet_poll_controller,
 #endif
@@ -1331,6 +1571,7 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
 	SET_NETDEV_DEV(netdev, &dev->dev);
 
 	np->netdev = netdev;
+	np->netfront_xdp_enabled = false;
 
 	netif_carrier_off(netdev);
 
@@ -1419,6 +1660,8 @@ static void xennet_disconnect_backend(struct netfront_info *info)
 		queue->rx_ring_ref = GRANT_INVALID_REF;
 		queue->tx.sring = NULL;
 		queue->rx.sring = NULL;
+
+		page_pool_destroy(queue->page_pool);
 	}
 }
 
@@ -1754,6 +1997,49 @@ static void xennet_destroy_queues(struct netfront_info *info)
 	info->queues = NULL;
 }
 
+static int xennet_create_page_pool(struct netfront_queue *queue)
+{
+	int err;
+	struct page_pool_params pp_params = {
+		.order = 0,
+		.flags = 0,
+		.pool_size = NET_RX_RING_SIZE,
+		.nid = NUMA_NO_NODE,
+		.dev = &queue->info->netdev->dev,
+		.offset = XDP_PACKET_HEADROOM,
+		.max_len = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM,
+	};
+
+	queue->page_pool = page_pool_create(&pp_params);
+	if (IS_ERR(queue->page_pool)) {
+		err = PTR_ERR(queue->page_pool);
+		queue->page_pool = NULL;
+		return err;
+	}
+
+	err = xdp_rxq_info_reg(&queue->xdp_rxq, queue->info->netdev,
+			       queue->id);
+	if (err) {
+		netdev_err(queue->info->netdev, "xdp_rxq_info_reg failed\n");
+		goto err_free_pp;
+	}
+
+	err = xdp_rxq_info_reg_mem_model(&queue->xdp_rxq,
+					 MEM_TYPE_PAGE_POOL, queue->page_pool);
+	if (err) {
+		netdev_err(queue->info->netdev, "xdp_rxq_info_reg_mem_model failed\n");
+		goto err_unregister_rxq;
+	}
+	return 0;
+
+err_unregister_rxq:
+	xdp_rxq_info_unreg(&queue->xdp_rxq);
+err_free_pp:
+	page_pool_destroy(queue->page_pool);
+	queue->page_pool = NULL;
+	return err;
+}
+
 static int xennet_create_queues(struct netfront_info *info,
 				unsigned int *num_queues)
 {
@@ -1779,6 +2065,14 @@ static int xennet_create_queues(struct netfront_info *info,
 			break;
 		}
 
+		/* use page pool recycling instead of buddy allocator */
+		ret = xennet_create_page_pool(queue);
+		if (ret < 0) {
+			dev_err(&info->xbdev->dev, "can't allocate page pool\n");
+			*num_queues = i;
+			return ret;
+		}
+
 		netif_napi_add(queue->info->netdev, &queue->napi,
 			       xennet_poll, 64);
 		if (netif_running(info->netdev))
@@ -1825,6 +2119,15 @@ static int talk_to_netback(struct xenbus_device *dev,
 		goto out_unlocked;
 	}
 
+	info->netback_has_xdp_headroom = xenbus_read_unsigned(info->xbdev->otherend,
+							      "feature-xdp-headroom", 0);
+	/* set the current xen-netfront xdp state */
+	err = talk_to_netback_xdp(info, info->netfront_xdp_enabled ?
+				  NETBACK_XDP_HEADROOM_ENABLE :
+				  NETBACK_XDP_HEADROOM_DISABLE);
+	if (err)
+		goto out_unlocked;
+
 	rtnl_lock();
 	if (info->queues)
 		xennet_destroy_queues(info);
@@ -1959,6 +2262,8 @@ static int xennet_connect(struct net_device *dev)
 	err = talk_to_netback(np->xbdev, np);
 	if (err)
 		return err;
+	if (np->netback_has_xdp_headroom)
+		pr_info("backend supports XDP headroom\n");
 
 	/* talk_to_netback() sets the correct number of queues */
 	num_queues = dev->real_num_tx_queues;
-- 
1.8.3.1

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Jürgen Groß]

On 11.05.20 12:22, Denis Kirjanov wrote:
> The patch adds a basic XDP processing to xen-netfront driver.
> 
> We ran an XDP program for an RX response received from netback
> driver. Also we request xen-netback to adjust data offset for
> bpf_xdp_adjust_head() header space for custom headers.
> 
> synchronization between frontend and backend parts is done
> by using xenbus state switching:
> Reconfiguring -> Reconfigured- > Connected
> 
> UDP packets drop rate using xdp program is around 310 kpps
> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.

I'm still not seeing proper synchronization between frontend and
backend when an XDP program is activated.

Consider the following:

1. XDP program is not active, so RX responses have no XDP headroom
2. netback has pushed one (or more) RX responses to the ring page
3. XDP program is being activated -> Reconfiguring
4. netback acknowledges, will add XDP headroom for following RX
    responses
5. netfront reads RX response (2.) without XDP headroom from ring page
6. boom!


Juergen

> 
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
>   drivers/net/Kconfig        |   1 +
>   drivers/net/xen-netfront.c | 317 ++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 312 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 25a8f93..45918ce 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -479,6 +479,7 @@ config XEN_NETDEV_FRONTEND
>   	tristate "Xen network device frontend driver"
>   	depends on XEN
>   	select XEN_XENBUS_FRONTEND
> +	select PAGE_POOL
>   	default y
>   	help
>   	  This driver provides support for Xen paravirtual network
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 482c6c8..33544c7 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -44,6 +44,9 @@
>   #include <linux/mm.h>
>   #include <linux/slab.h>
>   #include <net/ip.h>
> +#include <linux/bpf.h>
> +#include <net/page_pool.h>
> +#include <linux/bpf_trace.h>
>   
>   #include <xen/xen.h>
>   #include <xen/xenbus.h>
> @@ -102,6 +105,8 @@ struct netfront_queue {
>   	char name[QUEUE_NAME_SIZE]; /* DEVNAME-qN */
>   	struct netfront_info *info;
>   
> +	struct bpf_prog __rcu *xdp_prog;
> +
>   	struct napi_struct napi;
>   
>   	/* Split event channels support, tx_* == rx_* when using
> @@ -144,6 +149,9 @@ struct netfront_queue {
>   	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
>   	grant_ref_t gref_rx_head;
>   	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
> +
> +	struct page_pool *page_pool;
> +	struct xdp_rxq_info xdp_rxq;
>   };
>   
>   struct netfront_info {
> @@ -159,6 +167,10 @@ struct netfront_info {
>   	struct netfront_stats __percpu *rx_stats;
>   	struct netfront_stats __percpu *tx_stats;
>   
> +	/* XDP state */
> +	bool netback_has_xdp_headroom;
> +	bool netfront_xdp_enabled;
> +
>   	atomic_t rx_gso_checksum_fixup;
>   };
>   
> @@ -265,8 +277,8 @@ static struct sk_buff *xennet_alloc_one_rx_buffer(struct netfront_queue *queue)
>   	if (unlikely(!skb))
>   		return NULL;
>   
> -	page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
> -	if (!page) {
> +	page = page_pool_dev_alloc_pages(queue->page_pool);
> +	if (unlikely(!page)) {
>   		kfree_skb(skb);
>   		return NULL;
>   	}
> @@ -560,6 +572,64 @@ static u16 xennet_select_queue(struct net_device *dev, struct sk_buff *skb,
>   	return queue_idx;
>   }
>   
> +static int xennet_xdp_xmit_one(struct net_device *dev, struct xdp_frame *xdpf)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
> +	struct netfront_queue *queue = NULL;
> +	unsigned int num_queues = dev->real_num_tx_queues;
> +	unsigned long flags;
> +	int notify;
> +	struct xen_netif_tx_request *tx;
> +
> +	queue = &np->queues[smp_processor_id() % num_queues];
> +
> +	spin_lock_irqsave(&queue->tx_lock, flags);
> +
> +	tx = xennet_make_first_txreq(queue, NULL,
> +				     virt_to_page(xdpf->data),
> +				     offset_in_page(xdpf->data),
> +				     xdpf->len);
> +
> +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&queue->tx, notify);
> +	if (notify)
> +		notify_remote_via_irq(queue->tx_irq);
> +
> +	u64_stats_update_begin(&tx_stats->syncp);
> +	tx_stats->bytes += xdpf->len;
> +	tx_stats->packets++;
> +	u64_stats_update_end(&tx_stats->syncp);
> +
> +	xennet_tx_buf_gc(queue);
> +
> +	spin_unlock_irqrestore(&queue->tx_lock, flags);
> +	return 0;
> +}
> +
> +static int xennet_xdp_xmit(struct net_device *dev, int n,
> +			   struct xdp_frame **frames, u32 flags)
> +{
> +	int drops = 0;
> +	int i, err;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	for (i = 0; i < n; i++) {
> +		struct xdp_frame *xdpf = frames[i];
> +
> +		if (!xdpf)
> +			continue;
> +		err = xennet_xdp_xmit_one(dev, xdpf);
> +		if (err) {
> +			xdp_return_frame_rx_napi(xdpf);
> +			drops++;
> +		}
> +	}
> +
> +	return n - drops;
> +}
> +
>   #define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
>   
>   static netdev_tx_t xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -778,9 +848,56 @@ static int xennet_get_extras(struct netfront_queue *queue,
>   	return err;
>   }
>   
> +static u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata,
> +			  struct xen_netif_rx_response *rx, struct bpf_prog *prog,
> +			  struct xdp_buff *xdp, bool *need_xdp_flush)
> +{
> +	struct xdp_frame *xdpf;
> +	u32 len = rx->status;
> +	u32 act = XDP_PASS;
> +	int err;
> +
> +	xdp->data_hard_start = page_address(pdata);
> +	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> +	xdp_set_data_meta_invalid(xdp);
> +	xdp->data_end = xdp->data + len;
> +	xdp->rxq = &queue->xdp_rxq;
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_TX:
> +		get_page(pdata);
> +		xdpf = convert_to_xdp_frame(xdp);
> +		err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0);
> +		if (unlikely(err < 0))
> +			trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +	case XDP_REDIRECT:
> +		get_page(pdata);
> +		err = xdp_do_redirect(queue->info->netdev, xdp, prog);
> +		*need_xdp_flush = true;
> +		if (unlikely(err))
> +			trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +	case XDP_PASS:
> +	case XDP_DROP:
> +		break;
> +
> +	case XDP_ABORTED:
> +		trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +	}
> +
> +	return act;
> +}
> +
>   static int xennet_get_responses(struct netfront_queue *queue,
>   				struct netfront_rx_info *rinfo, RING_IDX rp,
> -				struct sk_buff_head *list)
> +				struct sk_buff_head *list,
> +				bool *need_xdp_flush)
>   {
>   	struct xen_netif_rx_response *rx = &rinfo->rx;
>   	struct xen_netif_extra_info *extras = rinfo->extras;
> @@ -792,6 +909,9 @@ static int xennet_get_responses(struct netfront_queue *queue,
>   	int slots = 1;
>   	int err = 0;
>   	unsigned long ret;
> +	struct bpf_prog *xdp_prog;
> +	struct xdp_buff xdp;
> +	u32 verdict;
>   
>   	if (rx->flags & XEN_NETRXF_extra_info) {
>   		err = xennet_get_extras(queue, extras, rp);
> @@ -827,9 +947,24 @@ static int xennet_get_responses(struct netfront_queue *queue,
>   
>   		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>   
> -		__skb_queue_tail(list, skb);
> -
> +		rcu_read_lock();
> +		xdp_prog = rcu_dereference(queue->xdp_prog);
> +		if (xdp_prog) {
> +			if (!(rx->flags & XEN_NETRXF_more_data)) {
> +				/* currently only a single page contains data */
> +				verdict = xennet_run_xdp(queue,
> +							 skb_frag_page(&skb_shinfo(skb)->frags[0]),
> +							 rx, xdp_prog, &xdp, need_xdp_flush);
> +				if (verdict != XDP_PASS)
> +					err = -EINVAL;
> +			} else {
> +				/* drop the frame */
> +				err = -EINVAL;
> +			}
> +		}
> +		rcu_read_unlock();
>   next:
> +		__skb_queue_tail(list, skb);
>   		if (!(rx->flags & XEN_NETRXF_more_data))
>   			break;
>   
> @@ -997,7 +1132,9 @@ static int xennet_poll(struct napi_struct *napi, int budget)
>   	struct sk_buff_head rxq;
>   	struct sk_buff_head errq;
>   	struct sk_buff_head tmpq;
> +	struct bpf_prog *xdp_prog;
>   	int err;
> +	bool need_xdp_flush = false;
>   
>   	spin_lock(&queue->rx_lock);
>   
> @@ -1010,11 +1147,17 @@ static int xennet_poll(struct napi_struct *napi, int budget)
>   
>   	i = queue->rx.rsp_cons;
>   	work_done = 0;
> +	rcu_read_lock();
>   	while ((i != rp) && (work_done < budget)) {
>   		memcpy(rx, RING_GET_RESPONSE(&queue->rx, i), sizeof(*rx));
>   		memset(extras, 0, sizeof(rinfo.extras));
>   
> -		err = xennet_get_responses(queue, &rinfo, rp, &tmpq);
> +		xdp_prog = rcu_dereference(queue->xdp_prog);
> +		if (xdp_prog)
> +			rx->offset = XDP_PACKET_HEADROOM;
> +
> +		err = xennet_get_responses(queue, &rinfo, rp, &tmpq,
> +					   &need_xdp_flush);
>   
>   		if (unlikely(err)) {
>   err:
> @@ -1060,6 +1203,9 @@ static int xennet_poll(struct napi_struct *napi, int budget)
>   		i = ++queue->rx.rsp_cons;
>   		work_done++;
>   	}
> +	if (need_xdp_flush)
> +		xdp_do_flush();
> +	rcu_read_unlock();
>   
>   	__skb_queue_purge(&errq);
>   
> @@ -1261,6 +1407,98 @@ static void xennet_poll_controller(struct net_device *dev)
>   }
>   #endif
>   
> +#define NETBACK_XDP_HEADROOM_DISABLE	0
> +#define NETBACK_XDP_HEADROOM_ENABLE	1
> +
> +static int talk_to_netback_xdp(struct netfront_info *np, int xdp)
> +{
> +	int err;
> +
> +	err = xenbus_printf(XBT_NIL, np->xbdev->nodename,
> +			    "feature-xdp", "%u", xdp);
> +	if (err)
> +		pr_debug("Error writing feature-xdp\n");
> +
> +	return err;
> +}
> +
> +static int xennet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> +			  struct netlink_ext_ack *extack)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	struct bpf_prog *old_prog;
> +	unsigned int i, err;
> +	unsigned long max_mtu = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM;
> +
> +	if (dev->mtu > max_mtu) {
> +		netdev_warn(dev, "XDP requires MTU less than %lu\n", max_mtu);
> +		return -EINVAL;
> +	}
> +
> +	if (!np->netback_has_xdp_headroom)
> +		return 0;
> +
> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
> +
> +	err = talk_to_netback_xdp(np, prog ? NETBACK_XDP_HEADROOM_ENABLE :
> +				  NETBACK_XDP_HEADROOM_DISABLE);
> +	if (err)
> +		return err;
> +
> +	/* avoid race with XDP headroom adjustment */
> +	wait_event(module_wq,
> +		   xenbus_read_driver_state(np->xbdev->otherend) ==
> +		   XenbusStateReconfigured);
> +	np->netfront_xdp_enabled = true;
> +
> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
> +
> +	if (prog)
> +		bpf_prog_add(prog, dev->real_num_tx_queues);
> +
> +	for (i = 0; i < dev->real_num_tx_queues; ++i)
> +		rcu_assign_pointer(np->queues[i].xdp_prog, prog);
> +
> +	if (old_prog)
> +		for (i = 0; i < dev->real_num_tx_queues; ++i)
> +			bpf_prog_put(old_prog);
> +
> +	xenbus_switch_state(np->xbdev, XenbusStateConnected);
> +
> +	return 0;
> +}
> +
> +static u32 xennet_xdp_query(struct net_device *dev)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	unsigned int num_queues = dev->real_num_tx_queues;
> +	unsigned int i;
> +	struct netfront_queue *queue;
> +	const struct bpf_prog *xdp_prog;
> +
> +	for (i = 0; i < num_queues; ++i) {
> +		queue = &np->queues[i];
> +		xdp_prog = rtnl_dereference(queue->xdp_prog);
> +		if (xdp_prog)
> +			return xdp_prog->aux->id;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xennet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> +{
> +	switch (xdp->command) {
> +	case XDP_SETUP_PROG:
> +		return xennet_xdp_set(dev, xdp->prog, xdp->extack);
> +	case XDP_QUERY_PROG:
> +		xdp->prog_id = xennet_xdp_query(dev);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>   static const struct net_device_ops xennet_netdev_ops = {
>   	.ndo_open            = xennet_open,
>   	.ndo_stop            = xennet_close,
> @@ -1272,6 +1510,8 @@ static void xennet_poll_controller(struct net_device *dev)
>   	.ndo_fix_features    = xennet_fix_features,
>   	.ndo_set_features    = xennet_set_features,
>   	.ndo_select_queue    = xennet_select_queue,
> +	.ndo_bpf            = xennet_xdp,
> +	.ndo_xdp_xmit	    = xennet_xdp_xmit,
>   #ifdef CONFIG_NET_POLL_CONTROLLER
>   	.ndo_poll_controller = xennet_poll_controller,
>   #endif
> @@ -1331,6 +1571,7 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
>   	SET_NETDEV_DEV(netdev, &dev->dev);
>   
>   	np->netdev = netdev;
> +	np->netfront_xdp_enabled = false;
>   
>   	netif_carrier_off(netdev);
>   
> @@ -1419,6 +1660,8 @@ static void xennet_disconnect_backend(struct netfront_info *info)
>   		queue->rx_ring_ref = GRANT_INVALID_REF;
>   		queue->tx.sring = NULL;
>   		queue->rx.sring = NULL;
> +
> +		page_pool_destroy(queue->page_pool);
>   	}
>   }
>   
> @@ -1754,6 +1997,49 @@ static void xennet_destroy_queues(struct netfront_info *info)
>   	info->queues = NULL;
>   }
>   
> +static int xennet_create_page_pool(struct netfront_queue *queue)
> +{
> +	int err;
> +	struct page_pool_params pp_params = {
> +		.order = 0,
> +		.flags = 0,
> +		.pool_size = NET_RX_RING_SIZE,
> +		.nid = NUMA_NO_NODE,
> +		.dev = &queue->info->netdev->dev,
> +		.offset = XDP_PACKET_HEADROOM,
> +		.max_len = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM,
> +	};
> +
> +	queue->page_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(queue->page_pool)) {
> +		err = PTR_ERR(queue->page_pool);
> +		queue->page_pool = NULL;
> +		return err;
> +	}
> +
> +	err = xdp_rxq_info_reg(&queue->xdp_rxq, queue->info->netdev,
> +			       queue->id);
> +	if (err) {
> +		netdev_err(queue->info->netdev, "xdp_rxq_info_reg failed\n");
> +		goto err_free_pp;
> +	}
> +
> +	err = xdp_rxq_info_reg_mem_model(&queue->xdp_rxq,
> +					 MEM_TYPE_PAGE_POOL, queue->page_pool);
> +	if (err) {
> +		netdev_err(queue->info->netdev, "xdp_rxq_info_reg_mem_model failed\n");
> +		goto err_unregister_rxq;
> +	}
> +	return 0;
> +
> +err_unregister_rxq:
> +	xdp_rxq_info_unreg(&queue->xdp_rxq);
> +err_free_pp:
> +	page_pool_destroy(queue->page_pool);
> +	queue->page_pool = NULL;
> +	return err;
> +}
> +
>   static int xennet_create_queues(struct netfront_info *info,
>   				unsigned int *num_queues)
>   {
> @@ -1779,6 +2065,14 @@ static int xennet_create_queues(struct netfront_info *info,
>   			break;
>   		}
>   
> +		/* use page pool recycling instead of buddy allocator */
> +		ret = xennet_create_page_pool(queue);
> +		if (ret < 0) {
> +			dev_err(&info->xbdev->dev, "can't allocate page pool\n");
> +			*num_queues = i;
> +			return ret;
> +		}
> +
>   		netif_napi_add(queue->info->netdev, &queue->napi,
>   			       xennet_poll, 64);
>   		if (netif_running(info->netdev))
> @@ -1825,6 +2119,15 @@ static int talk_to_netback(struct xenbus_device *dev,
>   		goto out_unlocked;
>   	}
>   
> +	info->netback_has_xdp_headroom = xenbus_read_unsigned(info->xbdev->otherend,
> +							      "feature-xdp-headroom", 0);
> +	/* set the current xen-netfront xdp state */
> +	err = talk_to_netback_xdp(info, info->netfront_xdp_enabled ?
> +				  NETBACK_XDP_HEADROOM_ENABLE :
> +				  NETBACK_XDP_HEADROOM_DISABLE);
> +	if (err)
> +		goto out_unlocked;
> +
>   	rtnl_lock();
>   	if (info->queues)
>   		xennet_destroy_queues(info);
> @@ -1959,6 +2262,8 @@ static int xennet_connect(struct net_device *dev)
>   	err = talk_to_netback(np->xbdev, np);
>   	if (err)
>   		return err;
> +	if (np->netback_has_xdp_headroom)
> +		pr_info("backend supports XDP headroom\n");
>   
>   	/* talk_to_netback() sets the correct number of queues */
>   	num_queues = dev->real_num_tx_queues;
> 

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Denis Kirjanov]

On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
> On 11.05.20 12:22, Denis Kirjanov wrote:
>> The patch adds a basic XDP processing to xen-netfront driver.
>>
>> We ran an XDP program for an RX response received from netback
>> driver. Also we request xen-netback to adjust data offset for
>> bpf_xdp_adjust_head() header space for custom headers.
>>
>> synchronization between frontend and backend parts is done
>> by using xenbus state switching:
>> Reconfiguring -> Reconfigured- > Connected
>>
>> UDP packets drop rate using xdp program is around 310 kpps
>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>
> I'm still not seeing proper synchronization between frontend and
> backend when an XDP program is activated.
>
> Consider the following:
>
> 1. XDP program is not active, so RX responses have no XDP headroom
> 2. netback has pushed one (or more) RX responses to the ring page
> 3. XDP program is being activated -> Reconfiguring
> 4. netback acknowledges, will add XDP headroom for following RX
>     responses
> 5. netfront reads RX response (2.) without XDP headroom from ring page
> 6. boom!

One thing that could be easily done is to set the offset on  xen-netback side
in  xenvif_rx_data_slot().  Are you okay with that?

If P

> Juergen
>
>>
>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> ---
>>   drivers/net/Kconfig        |   1 +
>>   drivers/net/xen-netfront.c | 317
>> ++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 312 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 25a8f93..45918ce 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -479,6 +479,7 @@ config XEN_NETDEV_FRONTEND
>>   	tristate "Xen network device frontend driver"
>>   	depends on XEN
>>   	select XEN_XENBUS_FRONTEND
>> +	select PAGE_POOL
>>   	default y
>>   	help
>>   	  This driver provides support for Xen paravirtual network
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index 482c6c8..33544c7 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
>> @@ -44,6 +44,9 @@
>>   #include <linux/mm.h>
>>   #include <linux/slab.h>
>>   #include <net/ip.h>
>> +#include <linux/bpf.h>
>> +#include <net/page_pool.h>
>> +#include <linux/bpf_trace.h>
>>
>>   #include <xen/xen.h>
>>   #include <xen/xenbus.h>
>> @@ -102,6 +105,8 @@ struct netfront_queue {
>>   	char name[QUEUE_NAME_SIZE]; /* DEVNAME-qN */
>>   	struct netfront_info *info;
>>
>> +	struct bpf_prog __rcu *xdp_prog;
>> +
>>   	struct napi_struct napi;
>>
>>   	/* Split event channels support, tx_* == rx_* when using
>> @@ -144,6 +149,9 @@ struct netfront_queue {
>>   	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
>>   	grant_ref_t gref_rx_head;
>>   	grant_ref_t grant_rx_ref[NET_RX_RING_SIZE];
>> +
>> +	struct page_pool *page_pool;
>> +	struct xdp_rxq_info xdp_rxq;
>>   };
>>
>>   struct netfront_info {
>> @@ -159,6 +167,10 @@ struct netfront_info {
>>   	struct netfront_stats __percpu *rx_stats;
>>   	struct netfront_stats __percpu *tx_stats;
>>
>> +	/* XDP state */
>> +	bool netback_has_xdp_headroom;
>> +	bool netfront_xdp_enabled;
>> +
>>   	atomic_t rx_gso_checksum_fixup;
>>   };
>>
>> @@ -265,8 +277,8 @@ static struct sk_buff
>> *xennet_alloc_one_rx_buffer(struct netfront_queue *queue)
>>   	if (unlikely(!skb))
>>   		return NULL;
>>
>> -	page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
>> -	if (!page) {
>> +	page = page_pool_dev_alloc_pages(queue->page_pool);
>> +	if (unlikely(!page)) {
>>   		kfree_skb(skb);
>>   		return NULL;
>>   	}
>> @@ -560,6 +572,64 @@ static u16 xennet_select_queue(struct net_device
>> *dev, struct sk_buff *skb,
>>   	return queue_idx;
>>   }
>>
>> +static int xennet_xdp_xmit_one(struct net_device *dev, struct xdp_frame
>> *xdpf)
>> +{
>> +	struct netfront_info *np = netdev_priv(dev);
>> +	struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
>> +	struct netfront_queue *queue = NULL;
>> +	unsigned int num_queues = dev->real_num_tx_queues;
>> +	unsigned long flags;
>> +	int notify;
>> +	struct xen_netif_tx_request *tx;
>> +
>> +	queue = &np->queues[smp_processor_id() % num_queues];
>> +
>> +	spin_lock_irqsave(&queue->tx_lock, flags);
>> +
>> +	tx = xennet_make_first_txreq(queue, NULL,
>> +				     virt_to_page(xdpf->data),
>> +				     offset_in_page(xdpf->data),
>> +				     xdpf->len);
>> +
>> +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&queue->tx, notify);
>> +	if (notify)
>> +		notify_remote_via_irq(queue->tx_irq);
>> +
>> +	u64_stats_update_begin(&tx_stats->syncp);
>> +	tx_stats->bytes += xdpf->len;
>> +	tx_stats->packets++;
>> +	u64_stats_update_end(&tx_stats->syncp);
>> +
>> +	xennet_tx_buf_gc(queue);
>> +
>> +	spin_unlock_irqrestore(&queue->tx_lock, flags);
>> +	return 0;
>> +}
>> +
>> +static int xennet_xdp_xmit(struct net_device *dev, int n,
>> +			   struct xdp_frame **frames, u32 flags)
>> +{
>> +	int drops = 0;
>> +	int i, err;
>> +
>> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < n; i++) {
>> +		struct xdp_frame *xdpf = frames[i];
>> +
>> +		if (!xdpf)
>> +			continue;
>> +		err = xennet_xdp_xmit_one(dev, xdpf);
>> +		if (err) {
>> +			xdp_return_frame_rx_napi(xdpf);
>> +			drops++;
>> +		}
>> +	}
>> +
>> +	return n - drops;
>> +}
>> +
>>   #define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
>>
>>   static netdev_tx_t xennet_start_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>> @@ -778,9 +848,56 @@ static int xennet_get_extras(struct netfront_queue
>> *queue,
>>   	return err;
>>   }
>>
>> +static u32 xennet_run_xdp(struct netfront_queue *queue, struct page
>> *pdata,
>> +			  struct xen_netif_rx_response *rx, struct bpf_prog *prog,
>> +			  struct xdp_buff *xdp, bool *need_xdp_flush)
>> +{
>> +	struct xdp_frame *xdpf;
>> +	u32 len = rx->status;
>> +	u32 act = XDP_PASS;
>> +	int err;
>> +
>> +	xdp->data_hard_start = page_address(pdata);
>> +	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
>> +	xdp_set_data_meta_invalid(xdp);
>> +	xdp->data_end = xdp->data + len;
>> +	xdp->rxq = &queue->xdp_rxq;
>> +
>> +	act = bpf_prog_run_xdp(prog, xdp);
>> +	switch (act) {
>> +	case XDP_TX:
>> +		get_page(pdata);
>> +		xdpf = convert_to_xdp_frame(xdp);
>> +		err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0);
>> +		if (unlikely(err < 0))
>> +			trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +	case XDP_REDIRECT:
>> +		get_page(pdata);
>> +		err = xdp_do_redirect(queue->info->netdev, xdp, prog);
>> +		*need_xdp_flush = true;
>> +		if (unlikely(err))
>> +			trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +	case XDP_PASS:
>> +	case XDP_DROP:
>> +		break;
>> +
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +
>> +	default:
>> +		bpf_warn_invalid_xdp_action(act);
>> +	}
>> +
>> +	return act;
>> +}
>> +
>>   static int xennet_get_responses(struct netfront_queue *queue,
>>   				struct netfront_rx_info *rinfo, RING_IDX rp,
>> -				struct sk_buff_head *list)
>> +				struct sk_buff_head *list,
>> +				bool *need_xdp_flush)
>>   {
>>   	struct xen_netif_rx_response *rx = &rinfo->rx;
>>   	struct xen_netif_extra_info *extras = rinfo->extras;
>> @@ -792,6 +909,9 @@ static int xennet_get_responses(struct netfront_queue
>> *queue,
>>   	int slots = 1;
>>   	int err = 0;
>>   	unsigned long ret;
>> +	struct bpf_prog *xdp_prog;
>> +	struct xdp_buff xdp;
>> +	u32 verdict;
>>
>>   	if (rx->flags & XEN_NETRXF_extra_info) {
>>   		err = xennet_get_extras(queue, extras, rp);
>> @@ -827,9 +947,24 @@ static int xennet_get_responses(struct netfront_queue
>> *queue,
>>
>>   		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>>
>> -		__skb_queue_tail(list, skb);
>> -
>> +		rcu_read_lock();
>> +		xdp_prog = rcu_dereference(queue->xdp_prog);
>> +		if (xdp_prog) {
>> +			if (!(rx->flags & XEN_NETRXF_more_data)) {
>> +				/* currently only a single page contains data */
>> +				verdict = xennet_run_xdp(queue,
>> +							 skb_frag_page(&skb_shinfo(skb)->frags[0]),
>> +							 rx, xdp_prog, &xdp, need_xdp_flush);
>> +				if (verdict != XDP_PASS)
>> +					err = -EINVAL;
>> +			} else {
>> +				/* drop the frame */
>> +				err = -EINVAL;
>> +			}
>> +		}
>> +		rcu_read_unlock();
>>   next:
>> +		__skb_queue_tail(list, skb);
>>   		if (!(rx->flags & XEN_NETRXF_more_data))
>>   			break;
>>
>> @@ -997,7 +1132,9 @@ static int xennet_poll(struct napi_struct *napi, int
>> budget)
>>   	struct sk_buff_head rxq;
>>   	struct sk_buff_head errq;
>>   	struct sk_buff_head tmpq;
>> +	struct bpf_prog *xdp_prog;
>>   	int err;
>> +	bool need_xdp_flush = false;
>>
>>   	spin_lock(&queue->rx_lock);
>>
>> @@ -1010,11 +1147,17 @@ static int xennet_poll(struct napi_struct *napi,
>> int budget)
>>
>>   	i = queue->rx.rsp_cons;
>>   	work_done = 0;
>> +	rcu_read_lock();
>>   	while ((i != rp) && (work_done < budget)) {
>>   		memcpy(rx, RING_GET_RESPONSE(&queue->rx, i), sizeof(*rx));
>>   		memset(extras, 0, sizeof(rinfo.extras));
>>
>> -		err = xennet_get_responses(queue, &rinfo, rp, &tmpq);
>> +		xdp_prog = rcu_dereference(queue->xdp_prog);
>> +		if (xdp_prog)
>> +			rx->offset = XDP_PACKET_HEADROOM;
>> +
>> +		err = xennet_get_responses(queue, &rinfo, rp, &tmpq,
>> +					   &need_xdp_flush);
>>
>>   		if (unlikely(err)) {
>>   err:
>> @@ -1060,6 +1203,9 @@ static int xennet_poll(struct napi_struct *napi, int
>> budget)
>>   		i = ++queue->rx.rsp_cons;
>>   		work_done++;
>>   	}
>> +	if (need_xdp_flush)
>> +		xdp_do_flush();
>> +	rcu_read_unlock();
>>
>>   	__skb_queue_purge(&errq);
>>
>> @@ -1261,6 +1407,98 @@ static void xennet_poll_controller(struct
>> net_device *dev)
>>   }
>>   #endif
>>
>> +#define NETBACK_XDP_HEADROOM_DISABLE	0
>> +#define NETBACK_XDP_HEADROOM_ENABLE	1
>> +
>> +static int talk_to_netback_xdp(struct netfront_info *np, int xdp)
>> +{
>> +	int err;
>> +
>> +	err = xenbus_printf(XBT_NIL, np->xbdev->nodename,
>> +			    "feature-xdp", "%u", xdp);
>> +	if (err)
>> +		pr_debug("Error writing feature-xdp\n");
>> +
>> +	return err;
>> +}
>> +
>> +static int xennet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> +			  struct netlink_ext_ack *extack)
>> +{
>> +	struct netfront_info *np = netdev_priv(dev);
>> +	struct bpf_prog *old_prog;
>> +	unsigned int i, err;
>> +	unsigned long max_mtu = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM;
>> +
>> +	if (dev->mtu > max_mtu) {
>> +		netdev_warn(dev, "XDP requires MTU less than %lu\n", max_mtu);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!np->netback_has_xdp_headroom)
>> +		return 0;
>> +
>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>> +
>> +	err = talk_to_netback_xdp(np, prog ? NETBACK_XDP_HEADROOM_ENABLE :
>> +				  NETBACK_XDP_HEADROOM_DISABLE);
>> +	if (err)
>> +		return err;
>> +
>> +	/* avoid race with XDP headroom adjustment */
>> +	wait_event(module_wq,
>> +		   xenbus_read_driver_state(np->xbdev->otherend) ==
>> +		   XenbusStateReconfigured);
>> +	np->netfront_xdp_enabled = true;
>> +
>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>> +
>> +	if (prog)
>> +		bpf_prog_add(prog, dev->real_num_tx_queues);
>> +
>> +	for (i = 0; i < dev->real_num_tx_queues; ++i)
>> +		rcu_assign_pointer(np->queues[i].xdp_prog, prog);
>> +
>> +	if (old_prog)
>> +		for (i = 0; i < dev->real_num_tx_queues; ++i)
>> +			bpf_prog_put(old_prog);
>> +
>> +	xenbus_switch_state(np->xbdev, XenbusStateConnected);
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 xennet_xdp_query(struct net_device *dev)
>> +{
>> +	struct netfront_info *np = netdev_priv(dev);
>> +	unsigned int num_queues = dev->real_num_tx_queues;
>> +	unsigned int i;
>> +	struct netfront_queue *queue;
>> +	const struct bpf_prog *xdp_prog;
>> +
>> +	for (i = 0; i < num_queues; ++i) {
>> +		queue = &np->queues[i];
>> +		xdp_prog = rtnl_dereference(queue->xdp_prog);
>> +		if (xdp_prog)
>> +			return xdp_prog->aux->id;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int xennet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>> +{
>> +	switch (xdp->command) {
>> +	case XDP_SETUP_PROG:
>> +		return xennet_xdp_set(dev, xdp->prog, xdp->extack);
>> +	case XDP_QUERY_PROG:
>> +		xdp->prog_id = xennet_xdp_query(dev);
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>   static const struct net_device_ops xennet_netdev_ops = {
>>   	.ndo_open            = xennet_open,
>>   	.ndo_stop            = xennet_close,
>> @@ -1272,6 +1510,8 @@ static void xennet_poll_controller(struct net_device
>> *dev)
>>   	.ndo_fix_features    = xennet_fix_features,
>>   	.ndo_set_features    = xennet_set_features,
>>   	.ndo_select_queue    = xennet_select_queue,
>> +	.ndo_bpf            = xennet_xdp,
>> +	.ndo_xdp_xmit	    = xennet_xdp_xmit,
>>   #ifdef CONFIG_NET_POLL_CONTROLLER
>>   	.ndo_poll_controller = xennet_poll_controller,
>>   #endif
>> @@ -1331,6 +1571,7 @@ static struct net_device *xennet_create_dev(struct
>> xenbus_device *dev)
>>   	SET_NETDEV_DEV(netdev, &dev->dev);
>>
>>   	np->netdev = netdev;
>> +	np->netfront_xdp_enabled = false;
>>
>>   	netif_carrier_off(netdev);
>>
>> @@ -1419,6 +1660,8 @@ static void xennet_disconnect_backend(struct
>> netfront_info *info)
>>   		queue->rx_ring_ref = GRANT_INVALID_REF;
>>   		queue->tx.sring = NULL;
>>   		queue->rx.sring = NULL;
>> +
>> +		page_pool_destroy(queue->page_pool);
>>   	}
>>   }
>>
>> @@ -1754,6 +1997,49 @@ static void xennet_destroy_queues(struct
>> netfront_info *info)
>>   	info->queues = NULL;
>>   }
>>
>> +static int xennet_create_page_pool(struct netfront_queue *queue)
>> +{
>> +	int err;
>> +	struct page_pool_params pp_params = {
>> +		.order = 0,
>> +		.flags = 0,
>> +		.pool_size = NET_RX_RING_SIZE,
>> +		.nid = NUMA_NO_NODE,
>> +		.dev = &queue->info->netdev->dev,
>> +		.offset = XDP_PACKET_HEADROOM,
>> +		.max_len = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM,
>> +	};
>> +
>> +	queue->page_pool = page_pool_create(&pp_params);
>> +	if (IS_ERR(queue->page_pool)) {
>> +		err = PTR_ERR(queue->page_pool);
>> +		queue->page_pool = NULL;
>> +		return err;
>> +	}
>> +
>> +	err = xdp_rxq_info_reg(&queue->xdp_rxq, queue->info->netdev,
>> +			       queue->id);
>> +	if (err) {
>> +		netdev_err(queue->info->netdev, "xdp_rxq_info_reg failed\n");
>> +		goto err_free_pp;
>> +	}
>> +
>> +	err = xdp_rxq_info_reg_mem_model(&queue->xdp_rxq,
>> +					 MEM_TYPE_PAGE_POOL, queue->page_pool);
>> +	if (err) {
>> +		netdev_err(queue->info->netdev, "xdp_rxq_info_reg_mem_model
>> failed\n");
>> +		goto err_unregister_rxq;
>> +	}
>> +	return 0;
>> +
>> +err_unregister_rxq:
>> +	xdp_rxq_info_unreg(&queue->xdp_rxq);
>> +err_free_pp:
>> +	page_pool_destroy(queue->page_pool);
>> +	queue->page_pool = NULL;
>> +	return err;
>> +}
>> +
>>   static int xennet_create_queues(struct netfront_info *info,
>>   				unsigned int *num_queues)
>>   {
>> @@ -1779,6 +2065,14 @@ static int xennet_create_queues(struct
>> netfront_info *info,
>>   			break;
>>   		}
>>
>> +		/* use page pool recycling instead of buddy allocator */
>> +		ret = xennet_create_page_pool(queue);
>> +		if (ret < 0) {
>> +			dev_err(&info->xbdev->dev, "can't allocate page pool\n");
>> +			*num_queues = i;
>> +			return ret;
>> +		}
>> +
>>   		netif_napi_add(queue->info->netdev, &queue->napi,
>>   			       xennet_poll, 64);
>>   		if (netif_running(info->netdev))
>> @@ -1825,6 +2119,15 @@ static int talk_to_netback(struct xenbus_device
>> *dev,
>>   		goto out_unlocked;
>>   	}
>>
>> +	info->netback_has_xdp_headroom =
>> xenbus_read_unsigned(info->xbdev->otherend,
>> +							      "feature-xdp-headroom", 0);
>> +	/* set the current xen-netfront xdp state */
>> +	err = talk_to_netback_xdp(info, info->netfront_xdp_enabled ?
>> +				  NETBACK_XDP_HEADROOM_ENABLE :
>> +				  NETBACK_XDP_HEADROOM_DISABLE);
>> +	if (err)
>> +		goto out_unlocked;
>> +
>>   	rtnl_lock();
>>   	if (info->queues)
>>   		xennet_destroy_queues(info);
>> @@ -1959,6 +2262,8 @@ static int xennet_connect(struct net_device *dev)
>>   	err = talk_to_netback(np->xbdev, np);
>>   	if (err)
>>   		return err;
>> +	if (np->netback_has_xdp_headroom)
>> +		pr_info("backend supports XDP headroom\n");
>>
>>   	/* talk_to_netback() sets the correct number of queues */
>>   	num_queues = dev->real_num_tx_queues;
>>
>
>

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Jürgen Groß]

On 11.05.20 19:27, Denis Kirjanov wrote:
> On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 11.05.20 12:22, Denis Kirjanov wrote:
>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>
>>> We ran an XDP program for an RX response received from netback
>>> driver. Also we request xen-netback to adjust data offset for
>>> bpf_xdp_adjust_head() header space for custom headers.
>>>
>>> synchronization between frontend and backend parts is done
>>> by using xenbus state switching:
>>> Reconfiguring -> Reconfigured- > Connected
>>>
>>> UDP packets drop rate using xdp program is around 310 kpps
>>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>>
>> I'm still not seeing proper synchronization between frontend and
>> backend when an XDP program is activated.
>>
>> Consider the following:
>>
>> 1. XDP program is not active, so RX responses have no XDP headroom
>> 2. netback has pushed one (or more) RX responses to the ring page
>> 3. XDP program is being activated -> Reconfiguring
>> 4. netback acknowledges, will add XDP headroom for following RX
>>      responses
>> 5. netfront reads RX response (2.) without XDP headroom from ring page
>> 6. boom!
> 
> One thing that could be easily done is to set the offset on  xen-netback side
> in  xenvif_rx_data_slot().  Are you okay with that?

How does this help in above case?

I think you haven't understood the problem I'm seeing.

There can be many RX responses in the ring page which haven't been
consumed by the frontend yet. You are doing the switch to XDP via a
different communication channel (Xenstore), so you need some way to
synchronize both communication channels.

Either you make sure you have read all RX responses before doing the
switch (this requires stopping netback to push out more RX responses),
or you need to have a flag in the RX responses indicating whether XDP
headroom is provided or not (requires an addition to the Xen netif
protocol). Or I'm completely wrong and this can not happen due to the
way XDP programs work, but you didn't provide any clear statement this
being the case.


Juergen

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Denis Kirjanov]

On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
> On 11.05.20 19:27, Denis Kirjanov wrote:
>> On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
>>> On 11.05.20 12:22, Denis Kirjanov wrote:
>>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>>
>>>> We ran an XDP program for an RX response received from netback
>>>> driver. Also we request xen-netback to adjust data offset for
>>>> bpf_xdp_adjust_head() header space for custom headers.
>>>>
>>>> synchronization between frontend and backend parts is done
>>>> by using xenbus state switching:
>>>> Reconfiguring -> Reconfigured- > Connected
>>>>
>>>> UDP packets drop rate using xdp program is around 310 kpps
>>>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>>>
>>> I'm still not seeing proper synchronization between frontend and
>>> backend when an XDP program is activated.
>>>
>>> Consider the following:
>>>
>>> 1. XDP program is not active, so RX responses have no XDP headroom
>>> 2. netback has pushed one (or more) RX responses to the ring page
>>> 3. XDP program is being activated -> Reconfiguring
>>> 4. netback acknowledges, will add XDP headroom for following RX
>>>      responses
>>> 5. netfront reads RX response (2.) without XDP headroom from ring page
>>> 6. boom!
>>
>> One thing that could be easily done is to set the offset on  xen-netback
>> side
>> in  xenvif_rx_data_slot().  Are you okay with that?
>
> How does this help in above case?
>
> I think you haven't understood the problem I'm seeing.
>
> There can be many RX responses in the ring page which haven't been
> consumed by the frontend yet. You are doing the switch to XDP via a
> different communication channel (Xenstore), so you need some way to
> synchronize both communication channels.
>
> Either you make sure you have read all RX responses before doing the
> switch (this requires stopping netback to push out more RX responses),
> or you need to have a flag in the RX responses indicating whether XDP
> headroom is provided or not (requires an addition to the Xen netif
> protocol).
Hi Jürgen,

I see your point that we can have a shared ring with mixed RX responses offset.
Since the offset field is set always  to 0 on netback side we can
adjust it and thus mark that a response has the offset adjusted or
it's not (if the offset filed is set to 0).

In this case we have to run an xdp program on netfront side only for a
response with offset set to xdp headroom.

I don't see a race in the scenario above.

Or I'm completely wrong and this can not happen due to the
> way XDP programs work, but you didn't provide any clear statement this
> being the case.
>
>
> Juergen
>

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Jürgen Groß]

On 12.05.20 14:27, Denis Kirjanov wrote:
> On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 11.05.20 19:27, Denis Kirjanov wrote:
>>> On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
>>>> On 11.05.20 12:22, Denis Kirjanov wrote:
>>>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>>>
>>>>> We ran an XDP program for an RX response received from netback
>>>>> driver. Also we request xen-netback to adjust data offset for
>>>>> bpf_xdp_adjust_head() header space for custom headers.
>>>>>
>>>>> synchronization between frontend and backend parts is done
>>>>> by using xenbus state switching:
>>>>> Reconfiguring -> Reconfigured- > Connected
>>>>>
>>>>> UDP packets drop rate using xdp program is around 310 kpps
>>>>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>>>>
>>>> I'm still not seeing proper synchronization between frontend and
>>>> backend when an XDP program is activated.
>>>>
>>>> Consider the following:
>>>>
>>>> 1. XDP program is not active, so RX responses have no XDP headroom
>>>> 2. netback has pushed one (or more) RX responses to the ring page
>>>> 3. XDP program is being activated -> Reconfiguring
>>>> 4. netback acknowledges, will add XDP headroom for following RX
>>>>       responses
>>>> 5. netfront reads RX response (2.) without XDP headroom from ring page
>>>> 6. boom!
>>>
>>> One thing that could be easily done is to set the offset on  xen-netback
>>> side
>>> in  xenvif_rx_data_slot().  Are you okay with that?
>>
>> How does this help in above case?
>>
>> I think you haven't understood the problem I'm seeing.
>>
>> There can be many RX responses in the ring page which haven't been
>> consumed by the frontend yet. You are doing the switch to XDP via a
>> different communication channel (Xenstore), so you need some way to
>> synchronize both communication channels.
>>
>> Either you make sure you have read all RX responses before doing the
>> switch (this requires stopping netback to push out more RX responses),
>> or you need to have a flag in the RX responses indicating whether XDP
>> headroom is provided or not (requires an addition to the Xen netif
>> protocol).
> Hi Jürgen,
> 
> I see your point that we can have a shared ring with mixed RX responses offset.
> Since the offset field is set always  to 0 on netback side we can
> adjust it and thus mark that a response has the offset adjusted or
> it's not (if the offset filed is set to 0).

For one I don't see your code in netfront to test this condition.

And I don't think this is a guaranteed interface. Have you checked all
netback versions in older kernels, in qemu, and in BSD?

BTW, I'm pretty sure the old xen-linux netback sometimes used an offset
not being 0. And yes, those kernels are still active in some cases (e.g.
SLES11-SP4 is still supported for customers having a long time service
agreement and this version is based on xen-linux).

> 
> In this case we have to run an xdp program on netfront side only for a
> response with offset set to xdp headroom.
> 
> I don't see a race in the scenario above.

I do.


Juergen

> 
> Or I'm completely wrong and this can not happen due to the
>> way XDP programs work, but you didn't provide any clear statement this
>> being the case.
>>
>>
>> Juergen
>>

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Denis Kirjanov]

On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
> On 12.05.20 14:27, Denis Kirjanov wrote:
>> On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
>>> On 11.05.20 19:27, Denis Kirjanov wrote:
>>>> On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>> On 11.05.20 12:22, Denis Kirjanov wrote:
>>>>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>>>>
>>>>>> We ran an XDP program for an RX response received from netback
>>>>>> driver. Also we request xen-netback to adjust data offset for
>>>>>> bpf_xdp_adjust_head() header space for custom headers.
>>>>>>
>>>>>> synchronization between frontend and backend parts is done
>>>>>> by using xenbus state switching:
>>>>>> Reconfiguring -> Reconfigured- > Connected
>>>>>>
>>>>>> UDP packets drop rate using xdp program is around 310 kpps
>>>>>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>>>>>
>>>>> I'm still not seeing proper synchronization between frontend and
>>>>> backend when an XDP program is activated.
>>>>>
>>>>> Consider the following:
>>>>>
>>>>> 1. XDP program is not active, so RX responses have no XDP headroom
>>>>> 2. netback has pushed one (or more) RX responses to the ring page
>>>>> 3. XDP program is being activated -> Reconfiguring
>>>>> 4. netback acknowledges, will add XDP headroom for following RX
>>>>>       responses
>>>>> 5. netfront reads RX response (2.) without XDP headroom from ring page
>>>>> 6. boom!
>>>>
>>>> One thing that could be easily done is to set the offset on
>>>> xen-netback
>>>> side
>>>> in  xenvif_rx_data_slot().  Are you okay with that?
>>>
>>> How does this help in above case?
>>>
>>> I think you haven't understood the problem I'm seeing.
>>>
>>> There can be many RX responses in the ring page which haven't been
>>> consumed by the frontend yet. You are doing the switch to XDP via a
>>> different communication channel (Xenstore), so you need some way to
>>> synchronize both communication channels.
>>>
>>> Either you make sure you have read all RX responses before doing the
>>> switch (this requires stopping netback to push out more RX responses),
>>> or you need to have a flag in the RX responses indicating whether XDP
>>> headroom is provided or not (requires an addition to the Xen netif
>>> protocol).
>> Hi Jürgen,
>>
>> I see your point that we can have a shared ring with mixed RX responses
>> offset.
>> Since the offset field is set always  to 0 on netback side we can
>> adjust it and thus mark that a response has the offset adjusted or
>> it's not (if the offset filed is set to 0).
>
> For one I don't see your code in netfront to test this condition.

Right, it's not in the current version.

>
> And I don't think this is a guaranteed interface. Have you checked all
> netback versions in older kernels, in qemu, and in BSD?
>
> BTW, I'm pretty sure the old xen-linux netback sometimes used an offset
> not being 0. And yes, those kernels are still active in some cases (e.g.
> SLES11-SP4 is still supported for customers having a long time service
> agreement and this version is based on xen-linux).

I see, good to know.
I think that I can add a new flag like XEN_NETRXF_xdp_headroom in this case

>>
>> In this case we have to run an xdp program on netfront side only for a
>> response with offset set to xdp headroom.
>>
>> I don't see a race in the scenario above.
>
> I do.
>
>
> Juergen
>
>>
>> Or I'm completely wrong and this can not happen due to the
>>> way XDP programs work, but you didn't provide any clear statement this
>>> being the case.
>>>
>>>
>>> Juergen
>>>
>
>

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[Jürgen Groß]

On 12.05.20 15:21, Denis Kirjanov wrote:
> On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 12.05.20 14:27, Denis Kirjanov wrote:
>>> On 5/12/20, Jürgen Groß <jgross@suse.com> wrote:
>>>> On 11.05.20 19:27, Denis Kirjanov wrote:
>>>>> On 5/11/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>>> On 11.05.20 12:22, Denis Kirjanov wrote:
>>>>>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>>>>>
>>>>>>> We ran an XDP program for an RX response received from netback
>>>>>>> driver. Also we request xen-netback to adjust data offset for
>>>>>>> bpf_xdp_adjust_head() header space for custom headers.
>>>>>>>
>>>>>>> synchronization between frontend and backend parts is done
>>>>>>> by using xenbus state switching:
>>>>>>> Reconfiguring -> Reconfigured- > Connected
>>>>>>>
>>>>>>> UDP packets drop rate using xdp program is around 310 kpps
>>>>>>> using ./pktgen_sample04_many_flows.sh and 160 kpps without the patch.
>>>>>>
>>>>>> I'm still not seeing proper synchronization between frontend and
>>>>>> backend when an XDP program is activated.
>>>>>>
>>>>>> Consider the following:
>>>>>>
>>>>>> 1. XDP program is not active, so RX responses have no XDP headroom
>>>>>> 2. netback has pushed one (or more) RX responses to the ring page
>>>>>> 3. XDP program is being activated -> Reconfiguring
>>>>>> 4. netback acknowledges, will add XDP headroom for following RX
>>>>>>        responses
>>>>>> 5. netfront reads RX response (2.) without XDP headroom from ring page
>>>>>> 6. boom!
>>>>>
>>>>> One thing that could be easily done is to set the offset on
>>>>> xen-netback
>>>>> side
>>>>> in  xenvif_rx_data_slot().  Are you okay with that?
>>>>
>>>> How does this help in above case?
>>>>
>>>> I think you haven't understood the problem I'm seeing.
>>>>
>>>> There can be many RX responses in the ring page which haven't been
>>>> consumed by the frontend yet. You are doing the switch to XDP via a
>>>> different communication channel (Xenstore), so you need some way to
>>>> synchronize both communication channels.
>>>>
>>>> Either you make sure you have read all RX responses before doing the
>>>> switch (this requires stopping netback to push out more RX responses),
>>>> or you need to have a flag in the RX responses indicating whether XDP
>>>> headroom is provided or not (requires an addition to the Xen netif
>>>> protocol).
>>> Hi Jürgen,
>>>
>>> I see your point that we can have a shared ring with mixed RX responses
>>> offset.
>>> Since the offset field is set always  to 0 on netback side we can
>>> adjust it and thus mark that a response has the offset adjusted or
>>> it's not (if the offset filed is set to 0).
>>
>> For one I don't see your code in netfront to test this condition.
> 
> Right, it's not in the current version.
> 
>>
>> And I don't think this is a guaranteed interface. Have you checked all
>> netback versions in older kernels, in qemu, and in BSD?
>>
>> BTW, I'm pretty sure the old xen-linux netback sometimes used an offset
>> not being 0. And yes, those kernels are still active in some cases (e.g.
>> SLES11-SP4 is still supported for customers having a long time service
>> agreement and this version is based on xen-linux).
> 
> I see, good to know.
> I think that I can add a new flag like XEN_NETRXF_xdp_headroom in this case

This will need to be defined in the related header in the Xen tree first
as this is the canonical source for all implementations of the Xen PV
network interface. See the file xen/include/public/io/netif.h in the Xen
tree (git://xenbits.xen.org/xen.git, patches should be sent to
xen-devel@lists.xenproject.org). When the interface change has been
approved I'd take the related change of the header in the Linux kernel.

This adds another question - is XDP_PACKET_HEADROOM guaranteed to always
have the same value across all implementations, or could it change e.g.
from one Linux kernel version to another, or are there other OS's with
XDP program support, possibly using a different value?

It might be a better idea to use a specific RX request for turning on
XDP support with the needed XDP_PACKET_HEADROOM value in it. This would
automatically solve the synchronization issue, as the related response
would automatically mark the synchronization point. And it would even
allow to have different headroom values in a netfront implementation
(maybe other OS's could use that capability).


Juergen

Re: [PATCH net-next v9 1/2] xen networking: add basic XDP support for xen-netfront

[David Miller]

From: Denis Kirjanov <kda@linux-powerpc.org>
Date: Mon, 11 May 2020 13:22:20 +0300

> @@ -560,6 +572,64 @@ static u16 xennet_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	return queue_idx;
>  }
>  
> +static int xennet_xdp_xmit_one(struct net_device *dev, struct xdp_frame *xdpf)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	struct netfront_stats *tx_stats = this_cpu_ptr(np->tx_stats);
> +	struct netfront_queue *queue = NULL;
> +	unsigned int num_queues = dev->real_num_tx_queues;
> +	unsigned long flags;
> +	int notify;
> +	struct xen_netif_tx_request *tx;

Reverse christmas tree ordering for these local variables please.

> @@ -792,6 +909,9 @@ static int xennet_get_responses(struct netfront_queue *queue,
>  	int slots = 1;
>  	int err = 0;
>  	unsigned long ret;
> +	struct bpf_prog *xdp_prog;
> +	struct xdp_buff xdp;
> +	u32 verdict;

Likewise.

> @@ -997,7 +1132,9 @@ static int xennet_poll(struct napi_struct *napi, int budget)
>  	struct sk_buff_head rxq;
>  	struct sk_buff_head errq;
>  	struct sk_buff_head tmpq;
> +	struct bpf_prog *xdp_prog;
>  	int err;
> +	bool need_xdp_flush = false;

Likewise.

> +static int xennet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> +			  struct netlink_ext_ack *extack)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	struct bpf_prog *old_prog;
> +	unsigned int i, err;
> +	unsigned long max_mtu = XEN_PAGE_SIZE - XDP_PACKET_HEADROOM;

Likewise.

> +static u32 xennet_xdp_query(struct net_device *dev)
> +{
> +	struct netfront_info *np = netdev_priv(dev);
> +	unsigned int num_queues = dev->real_num_tx_queues;
> +	unsigned int i;
> +	struct netfront_queue *queue;
> +	const struct bpf_prog *xdp_prog;

Likewise.

[PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Denis Kirjanov]

the patch basically adds the offset adjustment and netfront
state reading to make XDP work on netfront side.

Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
---
 drivers/net/xen-netback/common.h  |  2 ++
 drivers/net/xen-netback/netback.c |  7 +++++++
 drivers/net/xen-netback/rx.c      |  7 ++++++-
 drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 05847eb..4a148d6 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -280,6 +280,7 @@ struct xenvif {
 	u8 ip_csum:1;
 	u8 ipv6_csum:1;
 	u8 multicast_control:1;
+	u8 xdp_enabled:1;
 
 	/* Is this interface disabled? True when backend discovers
 	 * frontend is rogue.
@@ -395,6 +396,7 @@ static inline pending_ring_idx_t nr_pending_reqs(struct xenvif_queue *queue)
 irqreturn_t xenvif_interrupt(int irq, void *dev_id);
 
 extern bool separate_tx_rx_irq;
+extern bool provides_xdp_headroom;
 
 extern unsigned int rx_drain_timeout_msecs;
 extern unsigned int rx_stall_timeout_msecs;
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 315dfc6..6dfca72 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -96,6 +96,13 @@
 module_param_named(hash_cache_size, xenvif_hash_cache_size, uint, 0644);
 MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash cache");
 
+/* The module parameter tells that we have to put data
+ * for xen-netfront with the XDP_PACKET_HEADROOM offset
+ * needed for XDP processing
+ */
+bool provides_xdp_headroom = true;
+module_param(provides_xdp_headroom, bool, 0644);
+
 static void xenvif_idx_release(struct xenvif_queue *queue, u16 pending_idx,
 			       u8 status);
 
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index ef58870..c97c98e 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -33,6 +33,11 @@
 #include <xen/xen.h>
 #include <xen/events.h>
 
+static int xenvif_rx_xdp_offset(struct xenvif *vif)
+{
+	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
+}
+
 static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
 {
 	RING_IDX prod, cons;
@@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue *queue,
 				struct xen_netif_rx_request *req,
 				struct xen_netif_rx_response *rsp)
 {
-	unsigned int offset = 0;
+	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
 	unsigned int flags;
 
 	do {
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 286054b..d447191 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info *be,
 	}
 }
 
+static void read_xenbus_frontend_xdp(struct backend_info *be,
+				     struct xenbus_device *dev)
+{
+	struct xenvif *vif = be->vif;
+	unsigned int val;
+	int err;
+
+	err = xenbus_scanf(XBT_NIL, dev->otherend,
+			   "feature-xdp", "%u", &val);
+	if (err != 1)
+		return;
+	vif->xdp_enabled = val;
+}
+
 /**
  * Callback received when the frontend's state changes.
  */
@@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device *dev,
 		set_backend_state(be, XenbusStateConnected);
 		break;
 
+	case XenbusStateReconfiguring:
+		read_xenbus_frontend_xdp(be, dev);
+		xenbus_switch_state(dev, XenbusStateReconfigured);
+		break;
+
 	case XenbusStateClosing:
 		set_backend_state(be, XenbusStateClosing);
 		break;
@@ -1036,6 +1055,15 @@ static int netback_probe(struct xenbus_device *dev,
 			goto abort_transaction;
 		}
 
+		/* we can adjust a headroom for netfront XDP processing */
+		err = xenbus_printf(xbt, dev->nodename,
+				    "feature-xdp-headroom", "%d",
+				    provides_xdp_headroom);
+		if (err) {
+			message = "writing feature-xdp-headroom";
+			goto abort_transaction;
+		}
+
 		/* We don't support rx-flip path (except old guests who
 		 * don't grok this feature flag).
 		 */
-- 
1.8.3.1

RE: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Paul Durrant]

> -----Original Message-----
> From: Denis Kirjanov <kda@linux-powerpc.org>
> Sent: 11 May 2020 11:22
> To: netdev@vger.kernel.org
> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org; paul@xen.org; ilias.apalodimas@linaro.org
> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback
> 
> the patch basically adds the offset adjustment and netfront
> state reading to make XDP work on netfront side.
> 
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
>  drivers/net/xen-netback/common.h  |  2 ++
>  drivers/net/xen-netback/netback.c |  7 +++++++
>  drivers/net/xen-netback/rx.c      |  7 ++++++-
>  drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
>  4 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 05847eb..4a148d6 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -280,6 +280,7 @@ struct xenvif {
>  	u8 ip_csum:1;
>  	u8 ipv6_csum:1;
>  	u8 multicast_control:1;
> +	u8 xdp_enabled:1;
> 
>  	/* Is this interface disabled? True when backend discovers
>  	 * frontend is rogue.
> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t nr_pending_reqs(struct xenvif_queue *queue)
>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
> 
>  extern bool separate_tx_rx_irq;
> +extern bool provides_xdp_headroom;
> 
>  extern unsigned int rx_drain_timeout_msecs;
>  extern unsigned int rx_stall_timeout_msecs;
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 315dfc6..6dfca72 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -96,6 +96,13 @@
>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint, 0644);
>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash cache");
> 
> +/* The module parameter tells that we have to put data
> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
> + * needed for XDP processing
> + */
> +bool provides_xdp_headroom = true;
> +module_param(provides_xdp_headroom, bool, 0644);
> +
>  static void xenvif_idx_release(struct xenvif_queue *queue, u16 pending_idx,
>  			       u8 status);
> 
> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
> index ef58870..c97c98e 100644
> --- a/drivers/net/xen-netback/rx.c
> +++ b/drivers/net/xen-netback/rx.c
> @@ -33,6 +33,11 @@
>  #include <xen/xen.h>
>  #include <xen/events.h>
> 
> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
> +{
> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
> +}
> +
>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
>  {
>  	RING_IDX prod, cons;
> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue *queue,
>  				struct xen_netif_rx_request *req,
>  				struct xen_netif_rx_response *rsp)
>  {
> -	unsigned int offset = 0;
> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
>  	unsigned int flags;
> 
>  	do {
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 286054b..d447191 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info *be,
>  	}
>  }
> 
> +static void read_xenbus_frontend_xdp(struct backend_info *be,
> +				     struct xenbus_device *dev)
> +{
> +	struct xenvif *vif = be->vif;
> +	unsigned int val;
> +	int err;
> +
> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
> +			   "feature-xdp", "%u", &val);
> +	if (err != 1)
> +		return;
> +	vif->xdp_enabled = val;
> +}
> +
>  /**
>   * Callback received when the frontend's state changes.
>   */
> @@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device *dev,
>  		set_backend_state(be, XenbusStateConnected);
>  		break;
> 
> +	case XenbusStateReconfiguring:
> +		read_xenbus_frontend_xdp(be, dev);

I think this being the only call to read_xenbus_frontend_xdp() is still a problem. What happens if netback is reloaded against a
netfront that has already enabled 'feature-xdp'? AFAICT vif->xdp_enabled would remain false after the reload.

  Paul

> +		xenbus_switch_state(dev, XenbusStateReconfigured);
> +		break;
> +
>  	case XenbusStateClosing:
>  		set_backend_state(be, XenbusStateClosing);
>  		break;
> @@ -1036,6 +1055,15 @@ static int netback_probe(struct xenbus_device *dev,
>  			goto abort_transaction;
>  		}
> 
> +		/* we can adjust a headroom for netfront XDP processing */
> +		err = xenbus_printf(xbt, dev->nodename,
> +				    "feature-xdp-headroom", "%d",
> +				    provides_xdp_headroom);
> +		if (err) {
> +			message = "writing feature-xdp-headroom";
> +			goto abort_transaction;
> +		}
> +
>  		/* We don't support rx-flip path (except old guests who
>  		 * don't grok this feature flag).
>  		 */
> --
> 1.8.3.1

Re: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Denis Kirjanov]

On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> -----Original Message-----
>> From: Denis Kirjanov <kda@linux-powerpc.org>
>> Sent: 11 May 2020 11:22
>> To: netdev@vger.kernel.org
>> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org; paul@xen.org;
>> ilias.apalodimas@linaro.org
>> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment
>> to xen-netback
>>
>> the patch basically adds the offset adjustment and netfront
>> state reading to make XDP work on netfront side.
>>
>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> ---
>>  drivers/net/xen-netback/common.h  |  2 ++
>>  drivers/net/xen-netback/netback.c |  7 +++++++
>>  drivers/net/xen-netback/rx.c      |  7 ++++++-
>>  drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
>>  4 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/xen-netback/common.h
>> b/drivers/net/xen-netback/common.h
>> index 05847eb..4a148d6 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -280,6 +280,7 @@ struct xenvif {
>>  	u8 ip_csum:1;
>>  	u8 ipv6_csum:1;
>>  	u8 multicast_control:1;
>> +	u8 xdp_enabled:1;
>>
>>  	/* Is this interface disabled? True when backend discovers
>>  	 * frontend is rogue.
>> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t
>> nr_pending_reqs(struct xenvif_queue *queue)
>>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
>>
>>  extern bool separate_tx_rx_irq;
>> +extern bool provides_xdp_headroom;
>>
>>  extern unsigned int rx_drain_timeout_msecs;
>>  extern unsigned int rx_stall_timeout_msecs;
>> diff --git a/drivers/net/xen-netback/netback.c
>> b/drivers/net/xen-netback/netback.c
>> index 315dfc6..6dfca72 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -96,6 +96,13 @@
>>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint, 0644);
>>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash cache");
>>
>> +/* The module parameter tells that we have to put data
>> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
>> + * needed for XDP processing
>> + */
>> +bool provides_xdp_headroom = true;
>> +module_param(provides_xdp_headroom, bool, 0644);
>> +
>>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
>> pending_idx,
>>  			       u8 status);
>>
>> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
>> index ef58870..c97c98e 100644
>> --- a/drivers/net/xen-netback/rx.c
>> +++ b/drivers/net/xen-netback/rx.c
>> @@ -33,6 +33,11 @@
>>  #include <xen/xen.h>
>>  #include <xen/events.h>
>>
>> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
>> +{
>> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
>> +}
>> +
>>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
>>  {
>>  	RING_IDX prod, cons;
>> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue
>> *queue,
>>  				struct xen_netif_rx_request *req,
>>  				struct xen_netif_rx_response *rsp)
>>  {
>> -	unsigned int offset = 0;
>> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
>>  	unsigned int flags;
>>
>>  	do {
>> diff --git a/drivers/net/xen-netback/xenbus.c
>> b/drivers/net/xen-netback/xenbus.c
>> index 286054b..d447191 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info
>> *be,
>>  	}
>>  }
>>
>> +static void read_xenbus_frontend_xdp(struct backend_info *be,
>> +				     struct xenbus_device *dev)
>> +{
>> +	struct xenvif *vif = be->vif;
>> +	unsigned int val;
>> +	int err;
>> +
>> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +			   "feature-xdp", "%u", &val);
>> +	if (err != 1)
>> +		return;
>> +	vif->xdp_enabled = val;
>> +}
>> +
>>  /**
>>   * Callback received when the frontend's state changes.
>>   */
>> @@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device
>> *dev,
>>  		set_backend_state(be, XenbusStateConnected);
>>  		break;
>>
>> +	case XenbusStateReconfiguring:
>> +		read_xenbus_frontend_xdp(be, dev);
>
> I think this being the only call to read_xenbus_frontend_xdp() is still a
> problem. What happens if netback is reloaded against a
> netfront that has already enabled 'feature-xdp'? AFAICT vif->xdp_enabled
> would remain false after the reload.

in this case xennect_connect() should call talk_to_netback()
and the function will restore the state from info->netfront_xdp_enabled

>
>   Paul
>
>> +		xenbus_switch_state(dev, XenbusStateReconfigured);
>> +		break;
>> +
>>  	case XenbusStateClosing:
>>  		set_backend_state(be, XenbusStateClosing);
>>  		break;
>> @@ -1036,6 +1055,15 @@ static int netback_probe(struct xenbus_device
>> *dev,
>>  			goto abort_transaction;
>>  		}
>>
>> +		/* we can adjust a headroom for netfront XDP processing */
>> +		err = xenbus_printf(xbt, dev->nodename,
>> +				    "feature-xdp-headroom", "%d",
>> +				    provides_xdp_headroom);
>> +		if (err) {
>> +			message = "writing feature-xdp-headroom";
>> +			goto abort_transaction;
>> +		}
>> +
>>  		/* We don't support rx-flip path (except old guests who
>>  		 * don't grok this feature flag).
>>  		 */
>> --
>> 1.8.3.1
>
>
>

RE: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Paul Durrant]

> -----Original Message-----
> From: Denis Kirjanov <kda@linux-powerpc.org>
> Sent: 11 May 2020 13:12
> To: paul@xen.org
> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org;
> ilias.apalodimas@linaro.org
> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback
> 
> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
> >> -----Original Message-----
> >> From: Denis Kirjanov <kda@linux-powerpc.org>
> >> Sent: 11 May 2020 11:22
> >> To: netdev@vger.kernel.org
> >> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org; paul@xen.org;
> >> ilias.apalodimas@linaro.org
> >> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment
> >> to xen-netback
> >>
> >> the patch basically adds the offset adjustment and netfront
> >> state reading to make XDP work on netfront side.
> >>
> >> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> >> ---
> >>  drivers/net/xen-netback/common.h  |  2 ++
> >>  drivers/net/xen-netback/netback.c |  7 +++++++
> >>  drivers/net/xen-netback/rx.c      |  7 ++++++-
> >>  drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
> >>  4 files changed, 43 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/xen-netback/common.h
> >> b/drivers/net/xen-netback/common.h
> >> index 05847eb..4a148d6 100644
> >> --- a/drivers/net/xen-netback/common.h
> >> +++ b/drivers/net/xen-netback/common.h
> >> @@ -280,6 +280,7 @@ struct xenvif {
> >>  	u8 ip_csum:1;
> >>  	u8 ipv6_csum:1;
> >>  	u8 multicast_control:1;
> >> +	u8 xdp_enabled:1;
> >>
> >>  	/* Is this interface disabled? True when backend discovers
> >>  	 * frontend is rogue.
> >> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t
> >> nr_pending_reqs(struct xenvif_queue *queue)
> >>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
> >>
> >>  extern bool separate_tx_rx_irq;
> >> +extern bool provides_xdp_headroom;
> >>
> >>  extern unsigned int rx_drain_timeout_msecs;
> >>  extern unsigned int rx_stall_timeout_msecs;
> >> diff --git a/drivers/net/xen-netback/netback.c
> >> b/drivers/net/xen-netback/netback.c
> >> index 315dfc6..6dfca72 100644
> >> --- a/drivers/net/xen-netback/netback.c
> >> +++ b/drivers/net/xen-netback/netback.c
> >> @@ -96,6 +96,13 @@
> >>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint, 0644);
> >>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash cache");
> >>
> >> +/* The module parameter tells that we have to put data
> >> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
> >> + * needed for XDP processing
> >> + */
> >> +bool provides_xdp_headroom = true;
> >> +module_param(provides_xdp_headroom, bool, 0644);
> >> +
> >>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
> >> pending_idx,
> >>  			       u8 status);
> >>
> >> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
> >> index ef58870..c97c98e 100644
> >> --- a/drivers/net/xen-netback/rx.c
> >> +++ b/drivers/net/xen-netback/rx.c
> >> @@ -33,6 +33,11 @@
> >>  #include <xen/xen.h>
> >>  #include <xen/events.h>
> >>
> >> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
> >> +{
> >> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
> >> +}
> >> +
> >>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue *queue)
> >>  {
> >>  	RING_IDX prod, cons;
> >> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue
> >> *queue,
> >>  				struct xen_netif_rx_request *req,
> >>  				struct xen_netif_rx_response *rsp)
> >>  {
> >> -	unsigned int offset = 0;
> >> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
> >>  	unsigned int flags;
> >>
> >>  	do {
> >> diff --git a/drivers/net/xen-netback/xenbus.c
> >> b/drivers/net/xen-netback/xenbus.c
> >> index 286054b..d447191 100644
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info
> >> *be,
> >>  	}
> >>  }
> >>
> >> +static void read_xenbus_frontend_xdp(struct backend_info *be,
> >> +				     struct xenbus_device *dev)
> >> +{
> >> +	struct xenvif *vif = be->vif;
> >> +	unsigned int val;
> >> +	int err;
> >> +
> >> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
> >> +			   "feature-xdp", "%u", &val);
> >> +	if (err != 1)
> >> +		return;
> >> +	vif->xdp_enabled = val;
> >> +}
> >> +
> >>  /**
> >>   * Callback received when the frontend's state changes.
> >>   */
> >> @@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device
> >> *dev,
> >>  		set_backend_state(be, XenbusStateConnected);
> >>  		break;
> >>
> >> +	case XenbusStateReconfiguring:
> >> +		read_xenbus_frontend_xdp(be, dev);
> >
> > I think this being the only call to read_xenbus_frontend_xdp() is still a
> > problem. What happens if netback is reloaded against a
> > netfront that has already enabled 'feature-xdp'? AFAICT vif->xdp_enabled
> > would remain false after the reload.
> 
> in this case xennect_connect() should call talk_to_netback()
> and the function will restore the state from info->netfront_xdp_enabled
> 

No. You're assuming the frontend is aware the backend has been reloaded. It is not.

  Paul

Re: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Denis Kirjanov]

On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> -----Original Message-----
>> From: Denis Kirjanov <kda@linux-powerpc.org>
>> Sent: 11 May 2020 13:12
>> To: paul@xen.org
>> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com;
>> wei.liu@kernel.org;
>> ilias.apalodimas@linaro.org
>> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset
>> adjustment to xen-netback
>>
>> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> >> -----Original Message-----
>> >> From: Denis Kirjanov <kda@linux-powerpc.org>
>> >> Sent: 11 May 2020 11:22
>> >> To: netdev@vger.kernel.org
>> >> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org;
>> >> paul@xen.org;
>> >> ilias.apalodimas@linaro.org
>> >> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset
>> >> adjustment
>> >> to xen-netback
>> >>
>> >> the patch basically adds the offset adjustment and netfront
>> >> state reading to make XDP work on netfront side.
>> >>
>> >> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> >> ---
>> >>  drivers/net/xen-netback/common.h  |  2 ++
>> >>  drivers/net/xen-netback/netback.c |  7 +++++++
>> >>  drivers/net/xen-netback/rx.c      |  7 ++++++-
>> >>  drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
>> >>  4 files changed, 43 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/net/xen-netback/common.h
>> >> b/drivers/net/xen-netback/common.h
>> >> index 05847eb..4a148d6 100644
>> >> --- a/drivers/net/xen-netback/common.h
>> >> +++ b/drivers/net/xen-netback/common.h
>> >> @@ -280,6 +280,7 @@ struct xenvif {
>> >>  	u8 ip_csum:1;
>> >>  	u8 ipv6_csum:1;
>> >>  	u8 multicast_control:1;
>> >> +	u8 xdp_enabled:1;
>> >>
>> >>  	/* Is this interface disabled? True when backend discovers
>> >>  	 * frontend is rogue.
>> >> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t
>> >> nr_pending_reqs(struct xenvif_queue *queue)
>> >>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
>> >>
>> >>  extern bool separate_tx_rx_irq;
>> >> +extern bool provides_xdp_headroom;
>> >>
>> >>  extern unsigned int rx_drain_timeout_msecs;
>> >>  extern unsigned int rx_stall_timeout_msecs;
>> >> diff --git a/drivers/net/xen-netback/netback.c
>> >> b/drivers/net/xen-netback/netback.c
>> >> index 315dfc6..6dfca72 100644
>> >> --- a/drivers/net/xen-netback/netback.c
>> >> +++ b/drivers/net/xen-netback/netback.c
>> >> @@ -96,6 +96,13 @@
>> >>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint,
>> >> 0644);
>> >>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash
>> >> cache");
>> >>
>> >> +/* The module parameter tells that we have to put data
>> >> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
>> >> + * needed for XDP processing
>> >> + */
>> >> +bool provides_xdp_headroom = true;
>> >> +module_param(provides_xdp_headroom, bool, 0644);
>> >> +
>> >>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
>> >> pending_idx,
>> >>  			       u8 status);
>> >>
>> >> diff --git a/drivers/net/xen-netback/rx.c
>> >> b/drivers/net/xen-netback/rx.c
>> >> index ef58870..c97c98e 100644
>> >> --- a/drivers/net/xen-netback/rx.c
>> >> +++ b/drivers/net/xen-netback/rx.c
>> >> @@ -33,6 +33,11 @@
>> >>  #include <xen/xen.h>
>> >>  #include <xen/events.h>
>> >>
>> >> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
>> >> +{
>> >> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
>> >> +}
>> >> +
>> >>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue
>> >> *queue)
>> >>  {
>> >>  	RING_IDX prod, cons;
>> >> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct
>> >> xenvif_queue
>> >> *queue,
>> >>  				struct xen_netif_rx_request *req,
>> >>  				struct xen_netif_rx_response *rsp)
>> >>  {
>> >> -	unsigned int offset = 0;
>> >> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
>> >>  	unsigned int flags;
>> >>
>> >>  	do {
>> >> diff --git a/drivers/net/xen-netback/xenbus.c
>> >> b/drivers/net/xen-netback/xenbus.c
>> >> index 286054b..d447191 100644
>> >> --- a/drivers/net/xen-netback/xenbus.c
>> >> +++ b/drivers/net/xen-netback/xenbus.c
>> >> @@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info
>> >> *be,
>> >>  	}
>> >>  }
>> >>
>> >> +static void read_xenbus_frontend_xdp(struct backend_info *be,
>> >> +				     struct xenbus_device *dev)
>> >> +{
>> >> +	struct xenvif *vif = be->vif;
>> >> +	unsigned int val;
>> >> +	int err;
>> >> +
>> >> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> >> +			   "feature-xdp", "%u", &val);
>> >> +	if (err != 1)
>> >> +		return;
>> >> +	vif->xdp_enabled = val;
>> >> +}
>> >> +
>> >>  /**
>> >>   * Callback received when the frontend's state changes.
>> >>   */
>> >> @@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device
>> >> *dev,
>> >>  		set_backend_state(be, XenbusStateConnected);
>> >>  		break;
>> >>
>> >> +	case XenbusStateReconfiguring:
>> >> +		read_xenbus_frontend_xdp(be, dev);
>> >
>> > I think this being the only call to read_xenbus_frontend_xdp() is still
>> > a
>> > problem. What happens if netback is reloaded against a
>> > netfront that has already enabled 'feature-xdp'? AFAICT
>> > vif->xdp_enabled
>> > would remain false after the reload.
>>
>> in this case xennect_connect() should call talk_to_netback()
>> and the function will restore the state from info->netfront_xdp_enabled
>>
>
> No. You're assuming the frontend is aware the backend has been reloaded. It
> is not.

Hi Paul,

I've tried to unbind/bind the device and I can see that the variable
is set properly:

with enabled XDP:
<7>[  622.177935] xen_netback:backend_switch_state: backend/vif/2/0 -> InitWait
<7>[  622.179917] xen_netback:frontend_changed:
/local/domain/2/device/vif/0 -> Connected
<6>[  622.187393] vif vif-2-0 vif2.0: Guest Rx ready
<7>[  622.188451] xen_netback:backend_switch_state: backend/vif/2/0 -> Connected

localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
       feature-xdp-headroom = "1"
      feature-xdp = "1"

and with disabled:

7>[  758.216792] xen_netback:frontend_changed:
/local/domain/2/device/vif/0 -> Reconfiguring
<7>[  758.218741] xen_netback:frontend_changed:
/local/domain/2/device/vif/0 -> Connected
<7>[  784.177247] xen_netback:backend_switch_state: backend/vif/2/0 -> InitWait
<7>[  784.180101] xen_netback:frontend_changed:
/local/domain/2/device/vif/0 -> Connected
<6>[  784.187927] vif vif-2-0 vif2.0: Guest Rx ready
<7>[  784.188890] xen_netback:backend_switch_state: backend/vif/2/0 -> Connected

localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
       feature-xdp-headroom = "1"
      feature-xdp = "0"




>
>   Paul
>
>
>

RE: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Paul Durrant]

> -----Original Message-----
> From: Denis Kirjanov <kda@linux-powerpc.org>
> Sent: 11 May 2020 18:22
> To: paul@xen.org
> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org;
> ilias.apalodimas@linaro.org
> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback
> 
> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
> >> -----Original Message-----
> >> From: Denis Kirjanov <kda@linux-powerpc.org>
> >> Sent: 11 May 2020 13:12
> >> To: paul@xen.org
> >> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com;
> >> wei.liu@kernel.org;
> >> ilias.apalodimas@linaro.org
> >> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset
> >> adjustment to xen-netback
> >>
> >> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Denis Kirjanov <kda@linux-powerpc.org>
> >> >> Sent: 11 May 2020 11:22
> >> >> To: netdev@vger.kernel.org
> >> >> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org;
> >> >> paul@xen.org;
> >> >> ilias.apalodimas@linaro.org
> >> >> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset
> >> >> adjustment
> >> >> to xen-netback
> >> >>
> >> >> the patch basically adds the offset adjustment and netfront
> >> >> state reading to make XDP work on netfront side.
> >> >>
> >> >> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> >> >> ---
> >> >>  drivers/net/xen-netback/common.h  |  2 ++
> >> >>  drivers/net/xen-netback/netback.c |  7 +++++++
> >> >>  drivers/net/xen-netback/rx.c      |  7 ++++++-
> >> >>  drivers/net/xen-netback/xenbus.c  | 28 ++++++++++++++++++++++++++++
> >> >>  4 files changed, 43 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/net/xen-netback/common.h
> >> >> b/drivers/net/xen-netback/common.h
> >> >> index 05847eb..4a148d6 100644
> >> >> --- a/drivers/net/xen-netback/common.h
> >> >> +++ b/drivers/net/xen-netback/common.h
> >> >> @@ -280,6 +280,7 @@ struct xenvif {
> >> >>  	u8 ip_csum:1;
> >> >>  	u8 ipv6_csum:1;
> >> >>  	u8 multicast_control:1;
> >> >> +	u8 xdp_enabled:1;
> >> >>
> >> >>  	/* Is this interface disabled? True when backend discovers
> >> >>  	 * frontend is rogue.
> >> >> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t
> >> >> nr_pending_reqs(struct xenvif_queue *queue)
> >> >>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
> >> >>
> >> >>  extern bool separate_tx_rx_irq;
> >> >> +extern bool provides_xdp_headroom;
> >> >>
> >> >>  extern unsigned int rx_drain_timeout_msecs;
> >> >>  extern unsigned int rx_stall_timeout_msecs;
> >> >> diff --git a/drivers/net/xen-netback/netback.c
> >> >> b/drivers/net/xen-netback/netback.c
> >> >> index 315dfc6..6dfca72 100644
> >> >> --- a/drivers/net/xen-netback/netback.c
> >> >> +++ b/drivers/net/xen-netback/netback.c
> >> >> @@ -96,6 +96,13 @@
> >> >>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint,
> >> >> 0644);
> >> >>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash
> >> >> cache");
> >> >>
> >> >> +/* The module parameter tells that we have to put data
> >> >> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
> >> >> + * needed for XDP processing
> >> >> + */
> >> >> +bool provides_xdp_headroom = true;
> >> >> +module_param(provides_xdp_headroom, bool, 0644);
> >> >> +
> >> >>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
> >> >> pending_idx,
> >> >>  			       u8 status);
> >> >>
> >> >> diff --git a/drivers/net/xen-netback/rx.c
> >> >> b/drivers/net/xen-netback/rx.c
> >> >> index ef58870..c97c98e 100644
> >> >> --- a/drivers/net/xen-netback/rx.c
> >> >> +++ b/drivers/net/xen-netback/rx.c
> >> >> @@ -33,6 +33,11 @@
> >> >>  #include <xen/xen.h>
> >> >>  #include <xen/events.h>
> >> >>
> >> >> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
> >> >> +{
> >> >> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
> >> >> +}
> >> >> +
> >> >>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue
> >> >> *queue)
> >> >>  {
> >> >>  	RING_IDX prod, cons;
> >> >> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct
> >> >> xenvif_queue
> >> >> *queue,
> >> >>  				struct xen_netif_rx_request *req,
> >> >>  				struct xen_netif_rx_response *rsp)
> >> >>  {
> >> >> -	unsigned int offset = 0;
> >> >> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
> >> >>  	unsigned int flags;
> >> >>
> >> >>  	do {
> >> >> diff --git a/drivers/net/xen-netback/xenbus.c
> >> >> b/drivers/net/xen-netback/xenbus.c
> >> >> index 286054b..d447191 100644
> >> >> --- a/drivers/net/xen-netback/xenbus.c
> >> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> >> @@ -393,6 +393,20 @@ static void set_backend_state(struct backend_info
> >> >> *be,
> >> >>  	}
> >> >>  }
> >> >>
> >> >> +static void read_xenbus_frontend_xdp(struct backend_info *be,
> >> >> +				     struct xenbus_device *dev)
> >> >> +{
> >> >> +	struct xenvif *vif = be->vif;
> >> >> +	unsigned int val;
> >> >> +	int err;
> >> >> +
> >> >> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
> >> >> +			   "feature-xdp", "%u", &val);
> >> >> +	if (err != 1)
> >> >> +		return;
> >> >> +	vif->xdp_enabled = val;
> >> >> +}
> >> >> +
> >> >>  /**
> >> >>   * Callback received when the frontend's state changes.
> >> >>   */
> >> >> @@ -417,6 +431,11 @@ static void frontend_changed(struct xenbus_device
> >> >> *dev,
> >> >>  		set_backend_state(be, XenbusStateConnected);
> >> >>  		break;
> >> >>
> >> >> +	case XenbusStateReconfiguring:
> >> >> +		read_xenbus_frontend_xdp(be, dev);
> >> >
> >> > I think this being the only call to read_xenbus_frontend_xdp() is still
> >> > a
> >> > problem. What happens if netback is reloaded against a
> >> > netfront that has already enabled 'feature-xdp'? AFAICT
> >> > vif->xdp_enabled
> >> > would remain false after the reload.
> >>
> >> in this case xennect_connect() should call talk_to_netback()
> >> and the function will restore the state from info->netfront_xdp_enabled
> >>
> >
> > No. You're assuming the frontend is aware the backend has been reloaded. It
> > is not.
> 
> Hi Paul,
> 
> I've tried to unbind/bind the device and I can see that the variable
> is set properly:
> 
> with enabled XDP:
> <7>[  622.177935] xen_netback:backend_switch_state: backend/vif/2/0 -> InitWait
> <7>[  622.179917] xen_netback:frontend_changed:
> /local/domain/2/device/vif/0 -> Connected
> <6>[  622.187393] vif vif-2-0 vif2.0: Guest Rx ready
> <7>[  622.188451] xen_netback:backend_switch_state: backend/vif/2/0 -> Connected
> 
> localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
>        feature-xdp-headroom = "1"
>       feature-xdp = "1"
> 

So, that shows me the feature in xenstore. Has netback sampled it and set vif->xdp_enabled?

> and with disabled:
> 
> 7>[  758.216792] xen_netback:frontend_changed:
> /local/domain/2/device/vif/0 -> Reconfiguring

This I don't understand. What triggered a change of state in the frontend...

> <7>[  758.218741] xen_netback:frontend_changed:
> /local/domain/2/device/vif/0 -> Connected

...or did these lines occur before you bound netback?

  Paul

> <7>[  784.177247] xen_netback:backend_switch_state: backend/vif/2/0 -> InitWait
> <7>[  784.180101] xen_netback:frontend_changed:
> /local/domain/2/device/vif/0 -> Connected
> <6>[  784.187927] vif vif-2-0 vif2.0: Guest Rx ready
> <7>[  784.188890] xen_netback:backend_switch_state: backend/vif/2/0 -> Connected
> 
> localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
>        feature-xdp-headroom = "1"
>       feature-xdp = "0"
> 
> 
> 
> 
> >
> >   Paul
> >
> >
> >

Re: [PATCH net-next v9 2/2] xen networking: add XDP offset adjustment to xen-netback

[Denis Kirjanov]

On 5/12/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> -----Original Message-----
>> From: Denis Kirjanov <kda@linux-powerpc.org>
>> Sent: 11 May 2020 18:22
>> To: paul@xen.org
>> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com;
>> wei.liu@kernel.org;
>> ilias.apalodimas@linaro.org
>> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset
>> adjustment to xen-netback
>>
>> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> >> -----Original Message-----
>> >> From: Denis Kirjanov <kda@linux-powerpc.org>
>> >> Sent: 11 May 2020 13:12
>> >> To: paul@xen.org
>> >> Cc: netdev@vger.kernel.org; brouer@redhat.com; jgross@suse.com;
>> >> wei.liu@kernel.org;
>> >> ilias.apalodimas@linaro.org
>> >> Subject: Re: [PATCH net-next v9 2/2] xen networking: add XDP offset
>> >> adjustment to xen-netback
>> >>
>> >> On 5/11/20, Paul Durrant <xadimgnik@gmail.com> wrote:
>> >> >> -----Original Message-----
>> >> >> From: Denis Kirjanov <kda@linux-powerpc.org>
>> >> >> Sent: 11 May 2020 11:22
>> >> >> To: netdev@vger.kernel.org
>> >> >> Cc: brouer@redhat.com; jgross@suse.com; wei.liu@kernel.org;
>> >> >> paul@xen.org;
>> >> >> ilias.apalodimas@linaro.org
>> >> >> Subject: [PATCH net-next v9 2/2] xen networking: add XDP offset
>> >> >> adjustment
>> >> >> to xen-netback
>> >> >>
>> >> >> the patch basically adds the offset adjustment and netfront
>> >> >> state reading to make XDP work on netfront side.
>> >> >>
>> >> >> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> >> >> ---
>> >> >>  drivers/net/xen-netback/common.h  |  2 ++
>> >> >>  drivers/net/xen-netback/netback.c |  7 +++++++
>> >> >>  drivers/net/xen-netback/rx.c      |  7 ++++++-
>> >> >>  drivers/net/xen-netback/xenbus.c  | 28
>> >> >> ++++++++++++++++++++++++++++
>> >> >>  4 files changed, 43 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/net/xen-netback/common.h
>> >> >> b/drivers/net/xen-netback/common.h
>> >> >> index 05847eb..4a148d6 100644
>> >> >> --- a/drivers/net/xen-netback/common.h
>> >> >> +++ b/drivers/net/xen-netback/common.h
>> >> >> @@ -280,6 +280,7 @@ struct xenvif {
>> >> >>  	u8 ip_csum:1;
>> >> >>  	u8 ipv6_csum:1;
>> >> >>  	u8 multicast_control:1;
>> >> >> +	u8 xdp_enabled:1;
>> >> >>
>> >> >>  	/* Is this interface disabled? True when backend discovers
>> >> >>  	 * frontend is rogue.
>> >> >> @@ -395,6 +396,7 @@ static inline pending_ring_idx_t
>> >> >> nr_pending_reqs(struct xenvif_queue *queue)
>> >> >>  irqreturn_t xenvif_interrupt(int irq, void *dev_id);
>> >> >>
>> >> >>  extern bool separate_tx_rx_irq;
>> >> >> +extern bool provides_xdp_headroom;
>> >> >>
>> >> >>  extern unsigned int rx_drain_timeout_msecs;
>> >> >>  extern unsigned int rx_stall_timeout_msecs;
>> >> >> diff --git a/drivers/net/xen-netback/netback.c
>> >> >> b/drivers/net/xen-netback/netback.c
>> >> >> index 315dfc6..6dfca72 100644
>> >> >> --- a/drivers/net/xen-netback/netback.c
>> >> >> +++ b/drivers/net/xen-netback/netback.c
>> >> >> @@ -96,6 +96,13 @@
>> >> >>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint,
>> >> >> 0644);
>> >> >>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash
>> >> >> cache");
>> >> >>
>> >> >> +/* The module parameter tells that we have to put data
>> >> >> + * for xen-netfront with the XDP_PACKET_HEADROOM offset
>> >> >> + * needed for XDP processing
>> >> >> + */
>> >> >> +bool provides_xdp_headroom = true;
>> >> >> +module_param(provides_xdp_headroom, bool, 0644);
>> >> >> +
>> >> >>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
>> >> >> pending_idx,
>> >> >>  			       u8 status);
>> >> >>
>> >> >> diff --git a/drivers/net/xen-netback/rx.c
>> >> >> b/drivers/net/xen-netback/rx.c
>> >> >> index ef58870..c97c98e 100644
>> >> >> --- a/drivers/net/xen-netback/rx.c
>> >> >> +++ b/drivers/net/xen-netback/rx.c
>> >> >> @@ -33,6 +33,11 @@
>> >> >>  #include <xen/xen.h>
>> >> >>  #include <xen/events.h>
>> >> >>
>> >> >> +static int xenvif_rx_xdp_offset(struct xenvif *vif)
>> >> >> +{
>> >> >> +	return vif->xdp_enabled ? XDP_PACKET_HEADROOM : 0;
>> >> >> +}
>> >> >> +
>> >> >>  static bool xenvif_rx_ring_slots_available(struct xenvif_queue
>> >> >> *queue)
>> >> >>  {
>> >> >>  	RING_IDX prod, cons;
>> >> >> @@ -356,7 +361,7 @@ static void xenvif_rx_data_slot(struct
>> >> >> xenvif_queue
>> >> >> *queue,
>> >> >>  				struct xen_netif_rx_request *req,
>> >> >>  				struct xen_netif_rx_response *rsp)
>> >> >>  {
>> >> >> -	unsigned int offset = 0;
>> >> >> +	unsigned int offset = xenvif_rx_xdp_offset(queue->vif);
>> >> >>  	unsigned int flags;
>> >> >>
>> >> >>  	do {
>> >> >> diff --git a/drivers/net/xen-netback/xenbus.c
>> >> >> b/drivers/net/xen-netback/xenbus.c
>> >> >> index 286054b..d447191 100644
>> >> >> --- a/drivers/net/xen-netback/xenbus.c
>> >> >> +++ b/drivers/net/xen-netback/xenbus.c
>> >> >> @@ -393,6 +393,20 @@ static void set_backend_state(struct
>> >> >> backend_info
>> >> >> *be,
>> >> >>  	}
>> >> >>  }
>> >> >>
>> >> >> +static void read_xenbus_frontend_xdp(struct backend_info *be,
>> >> >> +				     struct xenbus_device *dev)
>> >> >> +{
>> >> >> +	struct xenvif *vif = be->vif;
>> >> >> +	unsigned int val;
>> >> >> +	int err;
>> >> >> +
>> >> >> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> >> >> +			   "feature-xdp", "%u", &val);
>> >> >> +	if (err != 1)
>> >> >> +		return;
>> >> >> +	vif->xdp_enabled = val;
>> >> >> +}
>> >> >> +
>> >> >>  /**
>> >> >>   * Callback received when the frontend's state changes.
>> >> >>   */
>> >> >> @@ -417,6 +431,11 @@ static void frontend_changed(struct
>> >> >> xenbus_device
>> >> >> *dev,
>> >> >>  		set_backend_state(be, XenbusStateConnected);
>> >> >>  		break;
>> >> >>
>> >> >> +	case XenbusStateReconfiguring:
>> >> >> +		read_xenbus_frontend_xdp(be, dev);
>> >> >
>> >> > I think this being the only call to read_xenbus_frontend_xdp() is
>> >> > still
>> >> > a
>> >> > problem. What happens if netback is reloaded against a
>> >> > netfront that has already enabled 'feature-xdp'? AFAICT
>> >> > vif->xdp_enabled
>> >> > would remain false after the reload.
>> >>
>> >> in this case xennect_connect() should call talk_to_netback()
>> >> and the function will restore the state from
>> >> info->netfront_xdp_enabled
>> >>
>> >
>> > No. You're assuming the frontend is aware the backend has been reloaded.
>> > It
>> > is not.
>>
>> Hi Paul,
>>
>> I've tried to unbind/bind the device and I can see that the variable
>> is set properly:
>>
>> with enabled XDP:
>> <7>[  622.177935] xen_netback:backend_switch_state: backend/vif/2/0 ->
>> InitWait
>> <7>[  622.179917] xen_netback:frontend_changed:
>> /local/domain/2/device/vif/0 -> Connected
>> <6>[  622.187393] vif vif-2-0 vif2.0: Guest Rx ready
>> <7>[  622.188451] xen_netback:backend_switch_state: backend/vif/2/0 ->
>> Connected
>>
>> localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
>>        feature-xdp-headroom = "1"
>>       feature-xdp = "1"
>>
>
> So, that shows me the feature in xenstore. Has netback sampled it and set
> vif->xdp_enabled?

right, what has to be additionally done is:

@@ -947,6 +967,8 @@ static int read_xenbus_vif_flags(struct backend_info *be)
        vif->ipv6_csum = !!xenbus_read_unsigned(dev->otherend,
                                                "feature-ipv6-csum-offload", 0);

+       read_xenbus_frontend_xdp(be, dev);
+
        return 0;
 }

>
>> and with disabled:
>>
>> 7>[  758.216792] xen_netback:frontend_changed:
>> /local/domain/2/device/vif/0 -> Reconfiguring
>
> This I don't understand. What triggered a change of state in the
> frontend...
>
>> <7>[  758.218741] xen_netback:frontend_changed:
>> /local/domain/2/device/vif/0 -> Connected
>
> ...or did these lines occur before you bound netback?
>
>   Paul
>
>> <7>[  784.177247] xen_netback:backend_switch_state: backend/vif/2/0 ->
>> InitWait
>> <7>[  784.180101] xen_netback:frontend_changed:
>> /local/domain/2/device/vif/0 -> Connected
>> <6>[  784.187927] vif vif-2-0 vif2.0: Guest Rx ready
>> <7>[  784.188890] xen_netback:backend_switch_state: backend/vif/2/0 ->
>> Connected
>>
>> localhost:/sys/bus/xen-backend/drivers/vif # xenstore-ls | grep xdp
>>        feature-xdp-headroom = "1"
>>       feature-xdp = "0"
>>
>>
>>
>>
>> >
>> >   Paul
>> >
>> >
>> >
>
>