[PATCH net-next v2] xen-netfront: add basic XDP support

[PATCH net-next v2] xen-netfront: add basic XDP support

[Denis Kirjanov]

the patch adds a basic xdo logic to the netfront driver

XDP redirect is not supported yet

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

Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
 drivers/net/xen-netback/common.h |   1 +
 drivers/net/xen-netback/rx.c     |   9 ++-
 drivers/net/xen-netback/xenbus.c |  21 ++++++
 drivers/net/xen-netfront.c       | 157 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 05847eb..0750c6f 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.
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index ef58870..a110a59 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 inline 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 {
@@ -389,7 +394,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue *queue,
 			flags |= XEN_NETRXF_extra_info;
 	}
 
-	rsp->offset = 0;
+	rsp->offset = xenvif_rx_xdp_offset(queue->vif);
 	rsp->flags = flags;
 	rsp->id = req->id;
 	rsp->status = (s16)offset;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 286054b..81a6023 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_fronetend_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 < 0)
+		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_fronetend_xdp(be, dev);
+		xenbus_switch_state(dev, XenbusStateReconfigured);
+		break;
+
 	case XenbusStateClosing:
 		set_backend_state(be, XenbusStateClosing);
 		break;
@@ -935,6 +954,8 @@ static int read_xenbus_vif_flags(struct backend_info *be)
 
 	vif->gso_mask = 0;
 
+	vif->xdp_enabled = 0;
+
 	if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
 		vif->gso_mask |= GSO_BIT(TCPV4);
 
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 482c6c8..db8a280 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -44,6 +44,8 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <net/ip.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -102,6 +104,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
@@ -778,6 +782,40 @@ static int xennet_get_extras(struct netfront_queue *queue,
 	return err;
 }
 
+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)
+{
+	u32 len = rx->status;
+	u32 act = XDP_PASS;
+
+	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->handle = 0;
+
+	act = bpf_prog_run_xdp(prog, xdp);
+	switch (act) {
+	case XDP_PASS:
+	case XDP_TX:
+	case XDP_DROP:
+		break;
+
+	case XDP_ABORTED:
+		trace_xdp_exception(queue->info->netdev, prog, act);
+		break;
+
+	default:
+		bpf_warn_invalid_xdp_action(act);
+	}
+
+	if (act != XDP_PASS && act != XDP_TX)
+		xdp->data_hard_start = NULL;
+
+	return act;
+}
+
 static int xennet_get_responses(struct netfront_queue *queue,
 				struct netfront_rx_info *rinfo, RING_IDX rp,
 				struct sk_buff_head *list)
@@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue *queue,
 
 		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
 
+		rcu_read_lock();
+		xdp_prog = rcu_dereference(queue->xdp_prog);
+		if (xdp_prog) {
+			/* currently only a single page contains data */
+			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
+			verdict = xennet_run_xdp(queue,
+				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
+				       rx, xdp_prog, &xdp);
+
+			if (verdict != XDP_PASS && verdict != XDP_TX) {
+				err = -EINVAL;
+				goto next;
+			}
+
+		}
+		rcu_read_unlock();
 		__skb_queue_tail(list, skb);
 
 next:
@@ -1261,6 +1318,105 @@ 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)
+{
+	struct xenbus_transaction xbt;
+	const char *message;
+	int err;
+
+again:
+	err = xenbus_transaction_start(&xbt);
+	if (err) {
+		xenbus_dev_fatal(np->xbdev, err, "starting transaction");
+		goto out;
+	}
+
+	err = xenbus_printf(xbt, np->xbdev->nodename, "feature-xdp", "%d", xdp);
+	if (err) {
+		message = "writing feature-xdp";
+		goto abort_transaction;
+	}
+
+	err = xenbus_transaction_end(xbt, 0);
+	if (err) {
+		if (err == -EAGAIN)
+			goto again;
+	}
+
+	return 0;
+
+abort_transaction:
+	xenbus_dev_fatal(np->xbdev, err, "%s", message);
+	xenbus_transaction_end(xbt, 1);
+out:
+	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;
+
+	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
+	if (!old_prog && !prog)
+		return 0;
+
+	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);
+
+	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
+				  NETBACK_XDP_HEADROOM_ENABLE);
+	if (err)
+		return err;
+
+	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
+
+	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 +1428,7 @@ 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,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = xennet_poll_controller,
 #endif
-- 
1.8.3.1

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jürgen Groß]

On 02.03.20 15:21, Denis Kirjanov wrote:
> the patch adds a basic xdo logic to the netfront driver
> 
> XDP redirect is not supported yet
> 
> v2:
> - avoid data copying while passing to XDP
> - tell xen-natback that we need the headroom space

Please add the patch history below the "---" delimiter

> 
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
>   drivers/net/xen-netback/common.h |   1 +
>   drivers/net/xen-netback/rx.c     |   9 ++-
>   drivers/net/xen-netback/xenbus.c |  21 ++++++
>   drivers/net/xen-netfront.c       | 157 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 186 insertions(+), 2 deletions(-)

You are modifying xen-netback sources. Please Cc the maintainers.

> 
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 05847eb..0750c6f 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.
> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
> index ef58870..a110a59 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 inline 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 {
> @@ -389,7 +394,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue *queue,
>   			flags |= XEN_NETRXF_extra_info;
>   	}
>   
> -	rsp->offset = 0;
> +	rsp->offset = xenvif_rx_xdp_offset(queue->vif);
>   	rsp->flags = flags;
>   	rsp->id = req->id;
>   	rsp->status = (s16)offset;
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 286054b..81a6023 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_fronetend_xdp(struct backend_info *be,
> +				      struct xenbus_device *dev)

Typo: s/fronetend/frontend/

> +{
> +	struct xenvif *vif = be->vif;
> +	unsigned int val;
> +	int err;
> +
> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
> +			   "feature-xdp", "%u", &val);
> +	if (err < 0)
> +		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_fronetend_xdp(be, dev);
> +		xenbus_switch_state(dev, XenbusStateReconfigured);
> +		break;
> +
Where is the reaction to the backend being set to "Reconfigured"?

>   	case XenbusStateClosing:
>   		set_backend_state(be, XenbusStateClosing);
>   		break;
> @@ -935,6 +954,8 @@ static int read_xenbus_vif_flags(struct backend_info *be)
>   
>   	vif->gso_mask = 0;
>   
> +	vif->xdp_enabled = 0;
> +
>   	if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
>   		vif->gso_mask |= GSO_BIT(TCPV4);
>   
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 482c6c8..db8a280 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -44,6 +44,8 @@
>   #include <linux/mm.h>
>   #include <linux/slab.h>
>   #include <net/ip.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_trace.h>
>   
>   #include <xen/xen.h>
>   #include <xen/xenbus.h>
> @@ -102,6 +104,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
> @@ -778,6 +782,40 @@ static int xennet_get_extras(struct netfront_queue *queue,
>   	return err;
>   }
>   
> +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)
> +{
> +	u32 len = rx->status;
> +	u32 act = XDP_PASS;
> +
> +	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->handle = 0;
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +	case XDP_TX:
> +	case XDP_DROP:
> +		break;
> +
> +	case XDP_ABORTED:
> +		trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +	}
> +
> +	if (act != XDP_PASS && act != XDP_TX)
> +		xdp->data_hard_start = NULL;
> +
> +	return act;
> +}
> +
>   static int xennet_get_responses(struct netfront_queue *queue,
>   				struct netfront_rx_info *rinfo, RING_IDX rp,
>   				struct sk_buff_head *list)
> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue *queue,
>   
>   		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>   
> +		rcu_read_lock();
> +		xdp_prog = rcu_dereference(queue->xdp_prog);
> +		if (xdp_prog) {
> +			/* currently only a single page contains data */
> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
> +			verdict = xennet_run_xdp(queue,
> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
> +				       rx, xdp_prog, &xdp);
> +
> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
> +				err = -EINVAL;
> +				goto next;
> +			}
> +
> +		}
> +		rcu_read_unlock();
>   		__skb_queue_tail(list, skb);
>   
>   next:
> @@ -1261,6 +1318,105 @@ 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)
> +{
> +	struct xenbus_transaction xbt;
> +	const char *message;
> +	int err;
> +
> +again:
> +	err = xenbus_transaction_start(&xbt);
> +	if (err) {
> +		xenbus_dev_fatal(np->xbdev, err, "starting transaction");
> +		goto out;
> +	}
> +
> +	err = xenbus_printf(xbt, np->xbdev->nodename, "feature-xdp", "%d", xdp);
> +	if (err) {
> +		message = "writing feature-xdp";
> +		goto abort_transaction;
> +	}
> +
> +	err = xenbus_transaction_end(xbt, 0);
> +	if (err) {
> +		if (err == -EAGAIN)
> +			goto again;

Writing a single node to Xenstore doesn't need a transaction.

> +	}
> +
> +	return 0;
> +
> +abort_transaction:
> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
> +	xenbus_transaction_end(xbt, 1);
> +out:
> +	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;
> +
> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
> +	if (!old_prog && !prog)
> +		return 0;
> +
> +	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);
> +
> +	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
> +				  NETBACK_XDP_HEADROOM_ENABLE);
> +	if (err)
> +		return err;
> +
> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);

What is happening in case the backend doesn't support XDP?

Is it really a good idea to communicate xdp_set via a frontend state
change? This will be rather slow. OTOH I have no idea how often this
might happen.


Juergen

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[David Miller]

From: Jürgen Groß <jgross@suse.com>
Date: Mon, 2 Mar 2020 16:29:41 +0100

> On 02.03.20 15:21, Denis Kirjanov wrote:
>> the patch adds a basic xdo logic to the netfront driver
>> XDP redirect is not supported yet
>> v2:
>> - avoid data copying while passing to XDP
>> - tell xen-natback that we need the headroom space
> 
> Please add the patch history below the "---" delimiter

Incorrect, I prefer to have them in the commit message and recorded
in the GIT history.

Please don't give out this advice for networking changes.

Thank you.

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Denis Kirjanov]

On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
> On 02.03.20 15:21, Denis Kirjanov wrote:
>> the patch adds a basic xdo logic to the netfront driver
>>
>> XDP redirect is not supported yet
>>
>> v2:
>> - avoid data copying while passing to XDP
>> - tell xen-natback that we need the headroom space
>
> Please add the patch history below the "---" delimiter
>
>>
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>> ---
>>   drivers/net/xen-netback/common.h |   1 +
>>   drivers/net/xen-netback/rx.c     |   9 ++-
>>   drivers/net/xen-netback/xenbus.c |  21 ++++++
>>   drivers/net/xen-netfront.c       | 157
>> +++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 186 insertions(+), 2 deletions(-)
>
> You are modifying xen-netback sources. Please Cc the maintainers.
>
>>
>> diff --git a/drivers/net/xen-netback/common.h
>> b/drivers/net/xen-netback/common.h
>> index 05847eb..0750c6f 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.
>> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
>> index ef58870..a110a59 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 inline 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 {
>> @@ -389,7 +394,7 @@ static void xenvif_rx_data_slot(struct xenvif_queue
>> *queue,
>>   			flags |= XEN_NETRXF_extra_info;
>>   	}
>>
>> -	rsp->offset = 0;
>> +	rsp->offset = xenvif_rx_xdp_offset(queue->vif);
>>   	rsp->flags = flags;
>>   	rsp->id = req->id;
>>   	rsp->status = (s16)offset;
>> diff --git a/drivers/net/xen-netback/xenbus.c
>> b/drivers/net/xen-netback/xenbus.c
>> index 286054b..81a6023 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_fronetend_xdp(struct backend_info *be,
>> +				      struct xenbus_device *dev)
>
> Typo: s/fronetend/frontend/
>
>> +{
>> +	struct xenvif *vif = be->vif;
>> +	unsigned int val;
>> +	int err;
>> +
>> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +			   "feature-xdp", "%u", &val);
>> +	if (err < 0)
>> +		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_fronetend_xdp(be, dev);
>> +		xenbus_switch_state(dev, XenbusStateReconfigured);
>> +		break;
>> +
> Where is the reaction to the backend being set to "Reconfigured"?
>
>>   	case XenbusStateClosing:
>>   		set_backend_state(be, XenbusStateClosing);
>>   		break;
>> @@ -935,6 +954,8 @@ static int read_xenbus_vif_flags(struct backend_info
>> *be)
>>
>>   	vif->gso_mask = 0;
>>
>> +	vif->xdp_enabled = 0;
>> +
>>   	if (xenbus_read_unsigned(dev->otherend, "feature-gso-tcpv4", 0))
>>   		vif->gso_mask |= GSO_BIT(TCPV4);
>>
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index 482c6c8..db8a280 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
>> @@ -44,6 +44,8 @@
>>   #include <linux/mm.h>
>>   #include <linux/slab.h>
>>   #include <net/ip.h>
>> +#include <linux/bpf.h>
>> +#include <linux/bpf_trace.h>
>>
>>   #include <xen/xen.h>
>>   #include <xen/xenbus.h>
>> @@ -102,6 +104,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
>> @@ -778,6 +782,40 @@ static int xennet_get_extras(struct netfront_queue
>> *queue,
>>   	return err;
>>   }
>>
>> +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)
>> +{
>> +	u32 len = rx->status;
>> +	u32 act = XDP_PASS;
>> +
>> +	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->handle = 0;
>> +
>> +	act = bpf_prog_run_xdp(prog, xdp);
>> +	switch (act) {
>> +	case XDP_PASS:
>> +	case XDP_TX:
>> +	case XDP_DROP:
>> +		break;
>> +
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +
>> +	default:
>> +		bpf_warn_invalid_xdp_action(act);
>> +	}
>> +
>> +	if (act != XDP_PASS && act != XDP_TX)
>> +		xdp->data_hard_start = NULL;
>> +
>> +	return act;
>> +}
>> +
>>   static int xennet_get_responses(struct netfront_queue *queue,
>>   				struct netfront_rx_info *rinfo, RING_IDX rp,
>>   				struct sk_buff_head *list)
>> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue
>> *queue,
>>
>>   		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>>
>> +		rcu_read_lock();
>> +		xdp_prog = rcu_dereference(queue->xdp_prog);
>> +		if (xdp_prog) {
>> +			/* currently only a single page contains data */
>> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
>> +			verdict = xennet_run_xdp(queue,
>> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
>> +				       rx, xdp_prog, &xdp);
>> +
>> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
>> +				err = -EINVAL;
>> +				goto next;
>> +			}
>> +
>> +		}
>> +		rcu_read_unlock();
>>   		__skb_queue_tail(list, skb);
>>
>>   next:
>> @@ -1261,6 +1318,105 @@ 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)
>> +{
>> +	struct xenbus_transaction xbt;
>> +	const char *message;
>> +	int err;
>> +
>> +again:
>> +	err = xenbus_transaction_start(&xbt);
>> +	if (err) {
>> +		xenbus_dev_fatal(np->xbdev, err, "starting transaction");
>> +		goto out;
>> +	}
>> +
>> +	err = xenbus_printf(xbt, np->xbdev->nodename, "feature-xdp", "%d",
>> xdp);
>> +	if (err) {
>> +		message = "writing feature-xdp";
>> +		goto abort_transaction;
>> +	}
>> +
>> +	err = xenbus_transaction_end(xbt, 0);
>> +	if (err) {
>> +		if (err == -EAGAIN)
>> +			goto again;
>
> Writing a single node to Xenstore doesn't need a transaction.
>
>> +	}
>> +
>> +	return 0;
>> +
>> +abort_transaction:
>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>> +	xenbus_transaction_end(xbt, 1);
>> +out:
>> +	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;
>> +
>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>> +	if (!old_prog && !prog)
>> +		return 0;
>> +
>> +	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);
>> +
>> +	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>> +	if (err)
>> +		return err;
>> +
>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>
> What is happening in case the backend doesn't support XDP?
Here we just ask xen-backend to make a headroom, that's it.
It's better to send xen-backend changes in a separate patch.

>
> Is it really a good idea to communicate xdp_set via a frontend state
> change? This will be rather slow. OTOH I have no idea how often this
> might happen.

I don't think that it's going to switch often and more likely it's a one shot
action.

>
>
> Juergen
>

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jürgen Groß]

On 04.03.20 14:10, Denis Kirjanov wrote:
> On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 02.03.20 15:21, Denis Kirjanov wrote:
>>> the patch adds a basic xdo logic to the netfront driver
>>>
>>> XDP redirect is not supported yet
>>>
>>> v2:
>>> - avoid data copying while passing to XDP
>>> - tell xen-natback that we need the headroom space
>>
>> Please add the patch history below the "---" delimiter
>>
>>>
>>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>>> ---
>>>    drivers/net/xen-netback/common.h |   1 +
>>>    drivers/net/xen-netback/rx.c     |   9 ++-
>>>    drivers/net/xen-netback/xenbus.c |  21 ++++++
>>>    drivers/net/xen-netfront.c       | 157
>>> +++++++++++++++++++++++++++++++++++++++
>>>    4 files changed, 186 insertions(+), 2 deletions(-)
>>
>> You are modifying xen-netback sources. Please Cc the maintainers.
>>

...

>>>
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +abort_transaction:
>>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>>> +	xenbus_transaction_end(xbt, 1);
>>> +out:
>>> +	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;
>>> +
>>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>>> +	if (!old_prog && !prog)
>>> +		return 0;
>>> +
>>> +	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);
>>> +
>>> +	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
>>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>>
>> What is happening in case the backend doesn't support XDP?
> Here we just ask xen-backend to make a headroom, that's it.
> It's better to send xen-backend changes in a separate patch.

Okay, but what do you do if the backend doesn't support XDP (e.g. in
case its an older kernel)? How do you know it is supporting XDP?

> 
>>
>> Is it really a good idea to communicate xdp_set via a frontend state
>> change? This will be rather slow. OTOH I have no idea how often this
>> might happen.
> 
> I don't think that it's going to switch often and more likely it's a one shot
> action.

What do you do in case of a live migration? You need to tell the backend
about XDP again.


Juergen

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Denis Kirjanov]

On 3/4/20, Jürgen Groß <jgross@suse.com> wrote:
> On 04.03.20 14:10, Denis Kirjanov wrote:
>> On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
>>> On 02.03.20 15:21, Denis Kirjanov wrote:
>>>> the patch adds a basic xdo logic to the netfront driver
>>>>
>>>> XDP redirect is not supported yet
>>>>
>>>> v2:
>>>> - avoid data copying while passing to XDP
>>>> - tell xen-natback that we need the headroom space
>>>
>>> Please add the patch history below the "---" delimiter
>>>
>>>>
>>>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>>>> ---
>>>>    drivers/net/xen-netback/common.h |   1 +
>>>>    drivers/net/xen-netback/rx.c     |   9 ++-
>>>>    drivers/net/xen-netback/xenbus.c |  21 ++++++
>>>>    drivers/net/xen-netfront.c       | 157
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 186 insertions(+), 2 deletions(-)
>>>
>>> You are modifying xen-netback sources. Please Cc the maintainers.
>>>
>
> ...
>
>>>>
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +abort_transaction:
>>>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>>>> +	xenbus_transaction_end(xbt, 1);
>>>> +out:
>>>> +	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;
>>>> +
>>>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>>>> +	if (!old_prog && !prog)
>>>> +		return 0;
>>>> +
>>>> +	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);
>>>> +
>>>> +	err = talk_to_netback_xdp(np, old_prog ?
>>>> NETBACK_XDP_HEADROOM_DISABLE:
>>>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>>>
>>> What is happening in case the backend doesn't support XDP?
>> Here we just ask xen-backend to make a headroom, that's it.
>> It's better to send xen-backend changes in a separate patch.
>
> Okay, but what do you do if the backend doesn't support XDP (e.g. in
> case its an older kernel)? How do you know it is supporting XDP?
We can check a xenbus reply to xenbus state change.

>
>>
>>>
>>> Is it really a good idea to communicate xdp_set via a frontend state
>>> change? This will be rather slow. OTOH I have no idea how often this
>>> might happen.
>>
>> I don't think that it's going to switch often and more likely it's a one
>> shot
>> action.
>
> What do you do in case of a live migration? You need to tell the backend
> about XDP again.

Yep I haven't thought about that. Thanks for pointing out.

>
>
> Juergen
>

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jürgen Groß]

On 05.03.20 10:47, Denis Kirjanov wrote:
> On 3/4/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 04.03.20 14:10, Denis Kirjanov wrote:
>>> On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
>>>> On 02.03.20 15:21, Denis Kirjanov wrote:
>>>>> the patch adds a basic xdo logic to the netfront driver
>>>>>
>>>>> XDP redirect is not supported yet
>>>>>
>>>>> v2:
>>>>> - avoid data copying while passing to XDP
>>>>> - tell xen-natback that we need the headroom space
>>>>
>>>> Please add the patch history below the "---" delimiter
>>>>
>>>>>
>>>>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>>>>> ---
>>>>>     drivers/net/xen-netback/common.h |   1 +
>>>>>     drivers/net/xen-netback/rx.c     |   9 ++-
>>>>>     drivers/net/xen-netback/xenbus.c |  21 ++++++
>>>>>     drivers/net/xen-netfront.c       | 157
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>     4 files changed, 186 insertions(+), 2 deletions(-)
>>>>
>>>> You are modifying xen-netback sources. Please Cc the maintainers.
>>>>
>>
>> ...
>>
>>>>>
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +abort_transaction:
>>>>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>>>>> +	xenbus_transaction_end(xbt, 1);
>>>>> +out:
>>>>> +	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;
>>>>> +
>>>>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>>>>> +	if (!old_prog && !prog)
>>>>> +		return 0;
>>>>> +
>>>>> +	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);
>>>>> +
>>>>> +	err = talk_to_netback_xdp(np, old_prog ?
>>>>> NETBACK_XDP_HEADROOM_DISABLE:
>>>>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>>>>
>>>> What is happening in case the backend doesn't support XDP?
>>> Here we just ask xen-backend to make a headroom, that's it.
>>> It's better to send xen-backend changes in a separate patch.
>>
>> Okay, but what do you do if the backend doesn't support XDP (e.g. in
>> case its an older kernel)? How do you know it is supporting XDP?
> We can check a xenbus reply to xenbus state change.

Using the frontend state for that purpose is rather dangerous.

In case the backend doesn't support the "Reconfiguring" state you might
end up with a broken network.

I'd rather let the backend advertise the support via a "feature-xdp"
node in Xenstore and do the activation via another node.


Juergen

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Ilias Apalodimas]

Hi Denis,

There's a bunch of things still missing from my remarks on V1.
XDP is not supposed to allocate and free pages constantly as that's one of the
things that's making it fast.

You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We usually
require the whole functionality to merge the driver.


On Mon, Mar 02, 2020 at 05:21:14PM +0300, Denis Kirjanov wrote:
>  
[...]
> +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)
> +{
> +	u32 len = rx->status;
> +	u32 act = XDP_PASS;
> +
> +	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->handle = 0;
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +	case XDP_TX:
> +	case XDP_DROP:

Maybe i am missing something on the usage, but XDP_TX and XDROP are handled
similarly?
XDP_TX is supposed to sent the packet out of the interface it arrived.

> +		break;
> +
> +	case XDP_ABORTED:
> +		trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +	}
> +
> +	if (act != XDP_PASS && act != XDP_TX)
> +		xdp->data_hard_start = NULL;
> +
> +	return act;
> +}
> +
>  static int xennet_get_responses(struct netfront_queue *queue,
>  				struct netfront_rx_info *rinfo, RING_IDX rp,
>  				struct sk_buff_head *list)
> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue *queue,
>  
>  		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>  
> +		rcu_read_lock();
> +		xdp_prog = rcu_dereference(queue->xdp_prog);
> +		if (xdp_prog) {
> +			/* currently only a single page contains data */
> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
> +			verdict = xennet_run_xdp(queue,
> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
> +				       rx, xdp_prog, &xdp);
> +
> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
> +				err = -EINVAL;
> +				goto next;
> +			}
> +
> +		}
> +		rcu_read_unlock();
>  		__skb_queue_tail(list, skb);
>  
>  next:
> @@ -1261,6 +1318,105 @@ 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)
> +{
> +	struct xenbus_transaction xbt;
> +	const char *message;
> +	int err;
> +
> +again:
> +	err = xenbus_transaction_start(&xbt);
> +	if (err) {
> +		xenbus_dev_fatal(np->xbdev, err, "starting transaction");
> +		goto out;
> +	}
> +
> +	err = xenbus_printf(xbt, np->xbdev->nodename, "feature-xdp", "%d", xdp);
> +	if (err) {
> +		message = "writing feature-xdp";
> +		goto abort_transaction;
> +	}
> +
> +	err = xenbus_transaction_end(xbt, 0);
> +	if (err) {
> +		if (err == -EAGAIN)
> +			goto again;
> +	}
> +
> +	return 0;
> +
> +abort_transaction:
> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
> +	xenbus_transaction_end(xbt, 1);
> +out:
> +	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;
> +
> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
> +	if (!old_prog && !prog)
> +		return 0;
> +
> +	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);
> +
> +	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
> +				  NETBACK_XDP_HEADROOM_ENABLE);
> +	if (err)
> +		return err;
> +
> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
> +
> +	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 +1428,7 @@ 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,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller = xennet_poll_controller,
>  #endif
> -- 
> 1.8.3.1
> 
Cheers
/Ilias

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Denis Kirjanov]

On 3/5/20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> Hi Denis,
>
> There's a bunch of things still missing from my remarks on V1.
> XDP is not supposed to allocate and free pages constantly as that's one of
> the
> things that's making it fast.

Hi Ilias,

I've removed the copying to an allocated page so there is no page
allocation/free logic added.


>
> You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We
> usually
> require the whole functionality to merge the driver.

I wanted to minimize changes and send follow-up patches

>
>
> On Mon, Mar 02, 2020 at 05:21:14PM +0300, Denis Kirjanov wrote:
>>
> [...]
>> +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)
>> +{
>> +	u32 len = rx->status;
>> +	u32 act = XDP_PASS;
>> +
>> +	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->handle = 0;
>> +
>> +	act = bpf_prog_run_xdp(prog, xdp);
>> +	switch (act) {
>> +	case XDP_PASS:
>> +	case XDP_TX:
>> +	case XDP_DROP:
>
> Maybe i am missing something on the usage, but XDP_TX and XDROP are handled
> similarly?
> XDP_TX is supposed to sent the packet out of the interface it arrived.

Yep, you're right. I'll add it to the next version.

Thanks!

>
>> +		break;
>> +
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +
>> +	default:
>> +		bpf_warn_invalid_xdp_action(act);
>> +	}
>> +
>> +	if (act != XDP_PASS && act != XDP_TX)
>> +		xdp->data_hard_start = NULL;
>> +
>> +	return act;
>> +}
>> +
>>  static int xennet_get_responses(struct netfront_queue *queue,
>>  				struct netfront_rx_info *rinfo, RING_IDX rp,
>>  				struct sk_buff_head *list)
>> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue
>> *queue,
>>
>>  		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>>
>> +		rcu_read_lock();
>> +		xdp_prog = rcu_dereference(queue->xdp_prog);
>> +		if (xdp_prog) {
>> +			/* currently only a single page contains data */
>> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
>> +			verdict = xennet_run_xdp(queue,
>> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
>> +				       rx, xdp_prog, &xdp);
>> +
>> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
>> +				err = -EINVAL;
>> +				goto next;
>> +			}
>> +
>> +		}
>> +		rcu_read_unlock();
>>  		__skb_queue_tail(list, skb);
>>
>>  next:
>> @@ -1261,6 +1318,105 @@ 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)
>> +{
>> +	struct xenbus_transaction xbt;
>> +	const char *message;
>> +	int err;
>> +
>> +again:
>> +	err = xenbus_transaction_start(&xbt);
>> +	if (err) {
>> +		xenbus_dev_fatal(np->xbdev, err, "starting transaction");
>> +		goto out;
>> +	}
>> +
>> +	err = xenbus_printf(xbt, np->xbdev->nodename, "feature-xdp", "%d",
>> xdp);
>> +	if (err) {
>> +		message = "writing feature-xdp";
>> +		goto abort_transaction;
>> +	}
>> +
>> +	err = xenbus_transaction_end(xbt, 0);
>> +	if (err) {
>> +		if (err == -EAGAIN)
>> +			goto again;
>> +	}
>> +
>> +	return 0;
>> +
>> +abort_transaction:
>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>> +	xenbus_transaction_end(xbt, 1);
>> +out:
>> +	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;
>> +
>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>> +	if (!old_prog && !prog)
>> +		return 0;
>> +
>> +	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);
>> +
>> +	err = talk_to_netback_xdp(np, old_prog ? NETBACK_XDP_HEADROOM_DISABLE:
>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>> +	if (err)
>> +		return err;
>> +
>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>> +
>> +	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 +1428,7 @@ 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,
>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>  	.ndo_poll_controller = xennet_poll_controller,
>>  #endif
>> --
>> 1.8.3.1
>>
> Cheers
> /Ilias
>


-- 
Regards / Mit besten Grüßen,
Denis

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Ilias Apalodimas]

On Thu, Mar 05, 2020 at 04:23:31PM +0300, Denis Kirjanov wrote:
> On 3/5/20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > Hi Denis,
> >
> > There's a bunch of things still missing from my remarks on V1.
> > XDP is not supposed to allocate and free pages constantly as that's one of
> > the
> > things that's making it fast.
> 
> Hi Ilias,
> 
> I've removed the copying to an allocated page so there is no page
> allocation/free logic added.
> 
i
Yea that has been removed. I am not familiar with the driver though, so i'll
give you an example. 
Let's say the BPF program says the packet must be dropped. What will happen to
the page with the packet payload?
Ideally on XDP we want that page recycled back into the device descriptors, so
the driver won't have to allocate and map a fresh page.

> 
> >
> > You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We
> > usually
> > require the whole functionality to merge the driver.
> 
> I wanted to minimize changes and send follow-up patches
> 

Adding XDP_REDIRECT is pretty trivial and the ndo_xdp_xmit should be very
similar to XDP_TX. So assuming you'll fix XDP_TX adding the .ndo one will be
relatively small amount of code.

> >
> >
> > On Mon, Mar 02, 2020 at 05:21:14PM +0300, Denis Kirjanov wrote:
> >>
> > [...]
> >> +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)
> >> +{
> >> +	u32 len = rx->status;
> >> +	u32 act = XDP_PASS;
> >> +
> >> +	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->handle = 0;
> >> +
> >> +	act = bpf_prog_run_xdp(prog, xdp);
> >> +	switch (act) {
> >> +	case XDP_PASS:
> >> +	case XDP_TX:
> >> +	case XDP_DROP:
> >
> > Maybe i am missing something on the usage, but XDP_TX and XDROP are handled
> > similarly?
> > XDP_TX is supposed to sent the packet out of the interface it arrived.
> 
> Yep, you're right. I'll add it to the next version.
> 
> Thanks!
> 

Cheers
/Ilias

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jesper Dangaard Brouer]

On Mon,  2 Mar 2020 17:21:14 +0300
Denis Kirjanov <kda@linux-powerpc.org> wrote:

> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 286054b..81a6023 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_fronetend_xdp(struct backend_info *be,
> +				      struct xenbus_device *dev)

Strange spelling of this function name: 'fronetend'

> +{
> +	struct xenvif *vif = be->vif;
> +	unsigned int val;
> +	int err;
> +
> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
> +			   "feature-xdp", "%u", &val);
> +	if (err < 0)
> +		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_fronetend_xdp(be, dev);
                            ^^^^^^^^
> +		xenbus_switch_state(dev, XenbusStateReconfigured);
> +		break;
> +

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jesper Dangaard Brouer]

On Mon,  2 Mar 2020 17:21:14 +0300
Denis Kirjanov <kda@linux-powerpc.org> wrote:

> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 482c6c8..db8a280 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
[...]
> @@ -778,6 +782,40 @@ static int xennet_get_extras(struct netfront_queue *queue,
>  	return err;
>  }
>  
> +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)
> +{
> +	u32 len = rx->status;
> +	u32 act = XDP_PASS;
> +
> +	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->handle = 0;
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +	case XDP_TX:
> +	case XDP_DROP:
> +		break;
> +
> +	case XDP_ABORTED:
> +		trace_xdp_exception(queue->info->netdev, prog, act);
> +		break;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +	}
> +
> +	if (act != XDP_PASS && act != XDP_TX)
> +		xdp->data_hard_start = NULL;
> +
> +	return act;
> +}
> +
>  static int xennet_get_responses(struct netfront_queue *queue,
>  				struct netfront_rx_info *rinfo, RING_IDX rp,
>  				struct sk_buff_head *list)
> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue *queue,
>  
>  		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>  
> +		rcu_read_lock();
> +		xdp_prog = rcu_dereference(queue->xdp_prog);
> +		if (xdp_prog) {
> +			/* currently only a single page contains data */
> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
> +			verdict = xennet_run_xdp(queue,
> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),

This looks really weird, skb_shinfo(skb)->frags[0], you already have an
SKB and you are sending fragment-0 to the xennet_run_xdp() function.

XDP meant to run before an SKB is allocated...

> +				       rx, xdp_prog, &xdp);
> +
> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
> +				err = -EINVAL;
> +				goto next;
> +			}
> +
> +		}
> +		rcu_read_unlock();
>  		__skb_queue_tail(list, skb);


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Jesper Dangaard Brouer]

On Thu, 5 Mar 2020 15:31:14 +0200
Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> On Thu, Mar 05, 2020 at 04:23:31PM +0300, Denis Kirjanov wrote:
> > On 3/5/20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:  
> > > Hi Denis,
> > >
> > > There's a bunch of things still missing from my remarks on V1.
> > > XDP is not supposed to allocate and free pages constantly as that's one of
> > > the things that's making it fast.  
> > 
> > Hi Ilias,
> > 
> > I've removed the copying to an allocated page so there is no page
> > allocation/free logic added.
> >   
> 
> Yea that has been removed. I am not familiar with the driver though, so i'll
> give you an example. 
> Let's say the BPF program says the packet must be dropped. What will happen to
> the page with the packet payload?
> Ideally on XDP we want that page recycled back into the device descriptors, so
> the driver won't have to allocate and map a fresh page.

I agree.  The main point with XDP is that we can do something
faster-than the normal network stack.  Especially in case of XDP_DROP,
we do driver specific recycling tricks, to avoid any allocations and
reinsert the RX-frame in RX-ring, and avoid overhead of SKB allocations.

Looking closer at your patch it seem you run XDP after the SKB alloc?!?

> >   
> > >
> > > You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We
> > > usually require the whole functionality to merge the driver.  

I agree, we have unfortunately seen drivers not getting completed if we
don't require full-XDP feature set.

> > 
> > I wanted to minimize changes and send follow-up patches
> >   
> 
> Adding XDP_REDIRECT is pretty trivial and the ndo_xdp_xmit should be very
> similar to XDP_TX. So assuming you'll fix XDP_TX adding the .ndo one will be
> relatively small amount of code.

You can have a patchset with more patches, if you prefer splitting this
up in multiple patches.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Denis Kirjanov]

On 3/5/20, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> On Mon,  2 Mar 2020 17:21:14 +0300
> Denis Kirjanov <kda@linux-powerpc.org> wrote:
>
>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>> index 482c6c8..db8a280 100644
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
> [...]
>> @@ -778,6 +782,40 @@ static int xennet_get_extras(struct netfront_queue
>> *queue,
>>  	return err;
>>  }
>>
>> +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)
>> +{
>> +	u32 len = rx->status;
>> +	u32 act = XDP_PASS;
>> +
>> +	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->handle = 0;
>> +
>> +	act = bpf_prog_run_xdp(prog, xdp);
>> +	switch (act) {
>> +	case XDP_PASS:
>> +	case XDP_TX:
>> +	case XDP_DROP:
>> +		break;
>> +
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(queue->info->netdev, prog, act);
>> +		break;
>> +
>> +	default:
>> +		bpf_warn_invalid_xdp_action(act);
>> +	}
>> +
>> +	if (act != XDP_PASS && act != XDP_TX)
>> +		xdp->data_hard_start = NULL;
>> +
>> +	return act;
>> +}
>> +
>>  static int xennet_get_responses(struct netfront_queue *queue,
>>  				struct netfront_rx_info *rinfo, RING_IDX rp,
>>  				struct sk_buff_head *list)
>> @@ -792,6 +830,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,6 +868,22 @@ static int xennet_get_responses(struct netfront_queue
>> *queue,
>>
>>  		gnttab_release_grant_reference(&queue->gref_rx_head, ref);
>>
>> +		rcu_read_lock();
>> +		xdp_prog = rcu_dereference(queue->xdp_prog);
>> +		if (xdp_prog) {
>> +			/* currently only a single page contains data */
>> +			WARN_ON_ONCE(skb_shinfo(skb)->nr_frags != 1);
>> +			verdict = xennet_run_xdp(queue,
>> +				       skb_frag_page(&skb_shinfo(skb)->frags[0]),
>
> This looks really weird, skb_shinfo(skb)->frags[0], you already have an
> SKB and you are sending fragment-0 to the xennet_run_xdp() function.
>
> XDP meant to run before an SKB is allocated...

Agreed. But the xen networking works that data communicated using
shared pages between backend and frontend parts.
So xen-netfront just allocates skbs with attached pages to them and
shares those pages with xen-netback

>
>> +				       rx, xdp_prog, &xdp);
>> +
>> +			if (verdict != XDP_PASS && verdict != XDP_TX) {
>> +				err = -EINVAL;
>> +				goto next;
>> +			}
>> +
>> +		}
>> +		rcu_read_unlock();
>>  		__skb_queue_tail(list, skb);
>
>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>
>

Re: [PATCH net-next v2] xen-netfront: add basic XDP support

[Wei Liu]

On Thu, Mar 05, 2020 at 11:33:48AM +0100, Jürgen Groß wrote:
> On 05.03.20 10:47, Denis Kirjanov wrote:
> > On 3/4/20, Jürgen Groß <jgross@suse.com> wrote:
> > > On 04.03.20 14:10, Denis Kirjanov wrote:
> > > > On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
> > > > > On 02.03.20 15:21, Denis Kirjanov wrote:
> > > > > > the patch adds a basic xdo logic to the netfront driver
> > > > > > 
> > > > > > XDP redirect is not supported yet
> > > > > > 
> > > > > > v2:
> > > > > > - avoid data copying while passing to XDP
> > > > > > - tell xen-natback that we need the headroom space
> > > > > 
> > > > > Please add the patch history below the "---" delimiter
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> > > > > > ---
> > > > > >     drivers/net/xen-netback/common.h |   1 +
> > > > > >     drivers/net/xen-netback/rx.c     |   9 ++-
> > > > > >     drivers/net/xen-netback/xenbus.c |  21 ++++++
> > > > > >     drivers/net/xen-netfront.c       | 157
> > > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > >     4 files changed, 186 insertions(+), 2 deletions(-)
> > > > > 
> > > > > You are modifying xen-netback sources. Please Cc the maintainers.
> > > > > 
> > > 
> > > ...
> > > 
> > > > > > 
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > +abort_transaction:
> > > > > > +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
> > > > > > +	xenbus_transaction_end(xbt, 1);
> > > > > > +out:
> > > > > > +	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;
> > > > > > +
> > > > > > +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
> > > > > > +	if (!old_prog && !prog)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	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);
> > > > > > +
> > > > > > +	err = talk_to_netback_xdp(np, old_prog ?
> > > > > > NETBACK_XDP_HEADROOM_DISABLE:
> > > > > > +				  NETBACK_XDP_HEADROOM_ENABLE);
> > > > > > +	if (err)
> > > > > > +		return err;
> > > > > > +
> > > > > > +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
> > > > > 
> > > > > What is happening in case the backend doesn't support XDP?
> > > > Here we just ask xen-backend to make a headroom, that's it.
> > > > It's better to send xen-backend changes in a separate patch.
> > > 
> > > Okay, but what do you do if the backend doesn't support XDP (e.g. in
> > > case its an older kernel)? How do you know it is supporting XDP?
> > We can check a xenbus reply to xenbus state change.
> 
> Using the frontend state for that purpose is rather dangerous.
> 
> In case the backend doesn't support the "Reconfiguring" state you might
> end up with a broken network.
> 
> I'd rather let the backend advertise the support via a "feature-xdp"
> node in Xenstore and do the activation via another node.

Yes, that's how feature negotiation is supposed to work. If XDP somehow
doesn't fit into this model, we would like to know why at the very
least.

Wei.

> 
> 
> Juergen