[RFC bpf-next 0/6] net: xsk: minor improvements around queue handling

[RFC bpf-next 0/6] net: xsk: minor improvements around queue handling

[Jakub Kicinski]

Hi!

This set tries to make the core take care of error checking for the
drivers.  In particular making sure that the AF_XDP UMEM is not installed
on queues which don't exist (or are disabled) and that changing queue
(AKA ethtool channel) count cannot disable queues with active AF_XDF
zero-copy sockets.

I'm sending as an RFC because I'm not entirely sure what the desired
behaviour is here.  Is it Okay to install AF_XDP on queues which don't
exist?  I presume not?  Are the AF_XDP queue_ids referring to TX queues
as well as RX queues in case of the driver?  I presume not?  Should
we try to prevent disabling queues which have non zero-copy sockets
installed as well? :S

Anyway, if any of those patches seem useful and reasonable, please let
me know I will repost as non-RFC.

Jakub Kicinski (6):
  net: update real_num_rx_queues even when !CONFIG_SYSFS
  xsk: refactor xdp_umem_assign_dev()
  xsk: don't allow umem replace at stack level
  xsk: don't allow installing UMEM beyond the number of queues
  ethtool: rename local variable max -> curr
  ethtool: don't allow disabling queues with umem installed

 include/linux/netdevice.h | 16 +++++++--
 net/core/ethtool.c        | 19 ++++++----
 net/xdp/xdp_umem.c        | 73 ++++++++++++++++++++++++---------------
 3 files changed, 71 insertions(+), 37 deletions(-)

-- 
2.17.1

[RFC bpf-next 1/6] net: update real_num_rx_queues even when !CONFIG_SYSFS

[Jakub Kicinski]

We used to depend on real_num_rx_queues as a upper bound for sanity
checks.  For AF_XDP it's useful if drivers can trust the stack never
to try to install UMEM for queues which are not configured.  Update
dev->real_num_rx_queues even without sysfs compiled, otherwise it
would always stay equal dev->num_rx_queues.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/netdevice.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c1295c7a452e..6717dc7e8fbf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3433,6 +3433,7 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq);
 static inline int netif_set_real_num_rx_queues(struct net_device *dev,
 						unsigned int rxq)
 {
+	dev->real_num_rx_queues = rxqs;
 	return 0;
 }
 #endif
-- 
2.17.1

[RFC bpf-next 2/6] xsk: refactor xdp_umem_assign_dev()

[Jakub Kicinski]

Return early and only take the ref on dev once there is no possibility
of failing.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 net/xdp/xdp_umem.c | 49 ++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index f47abb46c587..c199d66b5f3f 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -56,41 +56,34 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 	if (force_copy)
 		return 0;
 
-	dev_hold(dev);
+	if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
+		return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
 
-	if (dev->netdev_ops->ndo_bpf && dev->netdev_ops->ndo_xsk_async_xmit) {
-		bpf.command = XDP_QUERY_XSK_UMEM;
+	bpf.command = XDP_QUERY_XSK_UMEM;
 
-		rtnl_lock();
-		err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-		rtnl_unlock();
+	rtnl_lock();
+	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+	rtnl_unlock();
 
-		if (err) {
-			dev_put(dev);
-			return force_zc ? -ENOTSUPP : 0;
-		}
+	if (err)
+		return force_zc ? -ENOTSUPP : 0;
 
-		bpf.command = XDP_SETUP_XSK_UMEM;
-		bpf.xsk.umem = umem;
-		bpf.xsk.queue_id = queue_id;
+	bpf.command = XDP_SETUP_XSK_UMEM;
+	bpf.xsk.umem = umem;
+	bpf.xsk.queue_id = queue_id;
 
-		rtnl_lock();
-		err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-		rtnl_unlock();
+	rtnl_lock();
+	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
+	rtnl_unlock();
 
-		if (err) {
-			dev_put(dev);
-			return force_zc ? err : 0; /* fail or fallback */
-		}
-
-		umem->dev = dev;
-		umem->queue_id = queue_id;
-		umem->zc = true;
-		return 0;
-	}
+	if (err)
+		return force_zc ? err : 0; /* fail or fallback */
 
-	dev_put(dev);
-	return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
+	dev_hold(dev);
+	umem->dev = dev;
+	umem->queue_id = queue_id;
+	umem->zc = true;
+	return 0;
 }
 
 static void xdp_umem_clear_dev(struct xdp_umem *umem)
-- 
2.17.1

Re: [RFC bpf-next 2/6] xsk: refactor xdp_umem_assign_dev()

[Björn Töpel]

Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Return early and only take the ref on dev once there is no possibility
> of failing.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
>  net/xdp/xdp_umem.c | 49 ++++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index f47abb46c587..c199d66b5f3f 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -56,41 +56,34 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
>         if (force_copy)
>                 return 0;
>
> -       dev_hold(dev);
> +       if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
> +               return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
>
> -       if (dev->netdev_ops->ndo_bpf && dev->netdev_ops->ndo_xsk_async_xmit) {
> -               bpf.command = XDP_QUERY_XSK_UMEM;
> +       bpf.command = XDP_QUERY_XSK_UMEM;
>
> -               rtnl_lock();
> -               err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> -               rtnl_unlock();
> +       rtnl_lock();
> +       err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> +       rtnl_unlock();
>
> -               if (err) {
> -                       dev_put(dev);
> -                       return force_zc ? -ENOTSUPP : 0;
> -               }
> +       if (err)
> +               return force_zc ? -ENOTSUPP : 0;
>
> -               bpf.command = XDP_SETUP_XSK_UMEM;
> -               bpf.xsk.umem = umem;
> -               bpf.xsk.queue_id = queue_id;
> +       bpf.command = XDP_SETUP_XSK_UMEM;
> +       bpf.xsk.umem = umem;
> +       bpf.xsk.queue_id = queue_id;
>
> -               rtnl_lock();
> -               err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> -               rtnl_unlock();
> +       rtnl_lock();
> +       err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> +       rtnl_unlock();
>
> -               if (err) {
> -                       dev_put(dev);
> -                       return force_zc ? err : 0; /* fail or fallback */
> -               }
> -
> -               umem->dev = dev;
> -               umem->queue_id = queue_id;
> -               umem->zc = true;
> -               return 0;
> -       }
> +       if (err)
> +               return force_zc ? err : 0; /* fail or fallback */
>
> -       dev_put(dev);
> -       return force_zc ? -ENOTSUPP : 0; /* fail or fallback */
> +       dev_hold(dev);
> +       umem->dev = dev;
> +       umem->queue_id = queue_id;
> +       umem->zc = true;
> +       return 0;
>  }
>
>  static void xdp_umem_clear_dev(struct xdp_umem *umem)
> --
> 2.17.1
>

Much cleaner! Please spin this w/o the RFC tag.

Acked-by: Björn Töpel <bjorn.topel@gmail.com>


Björn

[RFC bpf-next 3/6] xsk: don't allow umem replace at stack level

[Jakub Kicinski]

Currently drivers have to check if they already have a umem
installed for a given queue and return an error if so.  Make
better use of XDP_QUERY_XSK_UMEM and move this functionality
to the core.

We need to keep rtnl across the calls now.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/netdevice.h |  7 ++++---
 net/xdp/xdp_umem.c        | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6717dc7e8fbf..a5a34f0fb485 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -872,10 +872,10 @@ struct netdev_bpf {
 		struct {
 			struct bpf_offloaded_map *offmap;
 		};
-		/* XDP_SETUP_XSK_UMEM */
+		/* XDP_QUERY_XSK_UMEM, XDP_SETUP_XSK_UMEM */
 		struct {
-			struct xdp_umem *umem;
-			u16 queue_id;
+			struct xdp_umem *umem; /* out for query*/
+			u16 queue_id; /* in for query */
 		} xsk;
 	};
 };
@@ -3566,6 +3566,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		      int fd, u32 flags);
 u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
 		    enum bpf_netdev_command cmd);
+int xdp_umem_query(struct net_device *dev, u16 queue_id);
 
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index c199d66b5f3f..911ca6d3cb5a 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -11,6 +11,8 @@
 #include <linux/slab.h>
 #include <linux/bpf.h>
 #include <linux/mm.h>
+#include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
 
 #include "xdp_umem.h"
 #include "xsk_queue.h"
@@ -40,6 +42,21 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
 	}
 }
 
+int xdp_umem_query(struct net_device *dev, u16 queue_id)
+{
+	struct netdev_bpf bpf;
+
+	ASSERT_RTNL();
+
+	memset(&bpf, 0, sizeof(bpf));
+	bpf.command = XDP_QUERY_XSK_UMEM;
+	bpf.xsk.queue_id = queue_id;
+
+	if (!dev->netdev_ops->ndo_bpf)
+		return 0;
+	return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
+}
+
 int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 			u32 queue_id, u16 flags)
 {
@@ -62,28 +79,30 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 	bpf.command = XDP_QUERY_XSK_UMEM;
 
 	rtnl_lock();
-	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-	rtnl_unlock();
-
-	if (err)
-		return force_zc ? -ENOTSUPP : 0;
+	err = xdp_umem_query(dev, queue_id);
+	if (err) {
+		err = err < 0 ? -ENOTSUPP : -EBUSY;
+		goto err_rtnl_unlock;
+	}
 
 	bpf.command = XDP_SETUP_XSK_UMEM;
 	bpf.xsk.umem = umem;
 	bpf.xsk.queue_id = queue_id;
 
-	rtnl_lock();
 	err = dev->netdev_ops->ndo_bpf(dev, &bpf);
-	rtnl_unlock();
-
 	if (err)
-		return force_zc ? err : 0; /* fail or fallback */
+		goto err_rtnl_unlock;
+	rtnl_unlock();
 
 	dev_hold(dev);
 	umem->dev = dev;
 	umem->queue_id = queue_id;
 	umem->zc = true;
 	return 0;
+
+err_rtnl_unlock:
+	rtnl_unlock();
+	return force_zc ? err : 0; /* fail or fallback */
 }
 
 static void xdp_umem_clear_dev(struct xdp_umem *umem)
-- 
2.17.1

Re: [RFC bpf-next 3/6] xsk: don't allow umem replace at stack level

[Björn Töpel]

Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Currently drivers have to check if they already have a umem
> installed for a given queue and return an error if so.  Make
> better use of XDP_QUERY_XSK_UMEM and move this functionality
> to the core.
>
> We need to keep rtnl across the calls now.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
>  include/linux/netdevice.h |  7 ++++---
>  net/xdp/xdp_umem.c        | 37 ++++++++++++++++++++++++++++---------
>  2 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 6717dc7e8fbf..a5a34f0fb485 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -872,10 +872,10 @@ struct netdev_bpf {
>                 struct {
>                         struct bpf_offloaded_map *offmap;
>                 };
> -               /* XDP_SETUP_XSK_UMEM */
> +               /* XDP_QUERY_XSK_UMEM, XDP_SETUP_XSK_UMEM */
>                 struct {
> -                       struct xdp_umem *umem;
> -                       u16 queue_id;
> +                       struct xdp_umem *umem; /* out for query*/
> +                       u16 queue_id; /* in for query */
>                 } xsk;
>         };
>  };
> @@ -3566,6 +3566,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>                       int fd, u32 flags);
>  u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
>                     enum bpf_netdev_command cmd);
> +int xdp_umem_query(struct net_device *dev, u16 queue_id);
>
>  int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
>  int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index c199d66b5f3f..911ca6d3cb5a 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/bpf.h>
>  #include <linux/mm.h>
> +#include <linux/netdevice.h>
> +#include <linux/rtnetlink.h>
>
>  #include "xdp_umem.h"
>  #include "xsk_queue.h"
> @@ -40,6 +42,21 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
>         }
>  }
>
> +int xdp_umem_query(struct net_device *dev, u16 queue_id)
> +{
> +       struct netdev_bpf bpf;
> +
> +       ASSERT_RTNL();
> +
> +       memset(&bpf, 0, sizeof(bpf));
> +       bpf.command = XDP_QUERY_XSK_UMEM;
> +       bpf.xsk.queue_id = queue_id;
> +
> +       if (!dev->netdev_ops->ndo_bpf)
> +               return 0;
> +       return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
> +}
> +
>  int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
>                         u32 queue_id, u16 flags)
>  {
> @@ -62,28 +79,30 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
>         bpf.command = XDP_QUERY_XSK_UMEM;
>
>         rtnl_lock();
> -       err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> -       rtnl_unlock();
> -
> -       if (err)
> -               return force_zc ? -ENOTSUPP : 0;
> +       err = xdp_umem_query(dev, queue_id);
> +       if (err) {
> +               err = err < 0 ? -ENOTSUPP : -EBUSY;
> +               goto err_rtnl_unlock;
> +       }
>
>         bpf.command = XDP_SETUP_XSK_UMEM;
>         bpf.xsk.umem = umem;
>         bpf.xsk.queue_id = queue_id;
>
> -       rtnl_lock();
>         err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> -       rtnl_unlock();
> -
>         if (err)
> -               return force_zc ? err : 0; /* fail or fallback */
> +               goto err_rtnl_unlock;
> +       rtnl_unlock();
>
>         dev_hold(dev);
>         umem->dev = dev;
>         umem->queue_id = queue_id;
>         umem->zc = true;
>         return 0;
> +
> +err_rtnl_unlock:
> +       rtnl_unlock();
> +       return force_zc ? err : 0; /* fail or fallback */
>  }
>
>  static void xdp_umem_clear_dev(struct xdp_umem *umem)
> --
> 2.17.1
>

Nice!

For a non-RFC version,

Acked-by: Björn Töpel <bjorn.topel@intel.com>

[RFC bpf-next 4/6] xsk: don't allow installing UMEM beyond the number of queues

[Jakub Kicinski]

Don't allow installing UMEM on queue IDs higher than real_num_rx_queues.
Note that the check in xsk_bind() is advisory at most, since it's done
without rtnl.  Besides from driver's perspective the UMEM queue ID really
only relates to RX queues.  TX real_num_tx_queues queues the driver
exposes are for SKBs.  AF_XDP zero-copy TX will most likely use separate
rings much like XDP_TX does.  So the AF_XDP's concept of queue indexes
only relates to RX queues at driver level.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 net/xdp/xdp_umem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 911ca6d3cb5a..8139a2c5c5ed 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -79,6 +79,11 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 	bpf.command = XDP_QUERY_XSK_UMEM;
 
 	rtnl_lock();
+	if (queue_id >= dev->real_num_rx_queues) {
+		err = -EINVAL;
+		goto err_rtnl_unlock;
+	}
+
 	err = xdp_umem_query(dev, queue_id);
 	if (err) {
 		err = err < 0 ? -ENOTSUPP : -EBUSY;
-- 
2.17.1

[RFC bpf-next 5/6] ethtool: rename local variable max -> curr

[Jakub Kicinski]

ethtool_set_channels() validates the config against driver's max
settings.  It retrieves the current config and stores it in a
variable called max.  This was okay when only max settings were
accessed but we will soon want to access current settings as
well, so calling the entire structure max makes the code less
readable.

While at it drop unnecessary parenthesis.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 net/core/ethtool.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c9993c6c2fd4..4d5093577fe6 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1743,7 +1743,7 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
 static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
-	struct ethtool_channels channels, max = { .cmd = ETHTOOL_GCHANNELS };
+	struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };
 	u32 max_rx_in_use = 0;
 
 	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
@@ -1752,13 +1752,13 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
-	dev->ethtool_ops->get_channels(dev, &max);
+	dev->ethtool_ops->get_channels(dev, &curr);
 
 	/* ensure new counts are within the maximums */
-	if ((channels.rx_count > max.max_rx) ||
-	    (channels.tx_count > max.max_tx) ||
-	    (channels.combined_count > max.max_combined) ||
-	    (channels.other_count > max.max_other))
+	if (channels.rx_count > curr.max_rx ||
+	    channels.tx_count > curr.max_tx ||
+	    channels.combined_count > curr.max_combined ||
+	    channels.other_count > curr.max_other)
 		return -EINVAL;
 
 	/* ensure the new Rx count fits within the configured Rx flow
-- 
2.17.1

[RFC bpf-next 6/6] ethtool: don't allow disabling queues with umem installed

[Jakub Kicinski]

We already check the RSS indirection table does not use queues
which would be disabled by channel reconfiguration.  Make sure
user does not try to disable queues which have a UMEM and zero-
-copy AF_XDP socket installed.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 include/linux/netdevice.h | 8 ++++++++
 net/core/ethtool.c        | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a5a34f0fb485..c0df40deec54 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3566,7 +3566,15 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		      int fd, u32 flags);
 u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
 		    enum bpf_netdev_command cmd);
+
+#if defined(CONFIG_XDP_SOCKETS)
 int xdp_umem_query(struct net_device *dev, u16 queue_id);
+#else
+static inline int xdp_umem_query(struct net_device *dev, u16 queue_id)
+{
+	return 0;
+}
+#endif
 
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 4d5093577fe6..a8e693de5b11 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1745,6 +1745,7 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 {
 	struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };
 	u32 max_rx_in_use = 0;
+	unsigned int i;
 
 	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
 		return -EOPNOTSUPP;
@@ -1768,6 +1769,12 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 	    (channels.combined_count + channels.rx_count) <= max_rx_in_use)
 	    return -EINVAL;
 
+	/* Disabling channels, query zero-copy AF_XDP sockets */
+	for (i = channels.combined_count + channels.rx_count;
+	     i < curr.combined_count + curr.rx_count; i++)
+		if (xdp_umem_query(dev, i) > 0)
+			return -EINVAL;
+
 	return dev->ethtool_ops->set_channels(dev, &channels);
 }
 
-- 
2.17.1

Re: [RFC bpf-next 0/6] net: xsk: minor improvements around queue handling

[Björn Töpel]

Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> Hi!
>

Thanks for spending your time on this, Jakub. I'm (temporarily) back
for a week, so you can expect faster replies now...

> This set tries to make the core take care of error checking for the
> drivers.  In particular making sure that the AF_XDP UMEM is not installed
> on queues which don't exist (or are disabled) and that changing queue
> (AKA ethtool channel) count cannot disable queues with active AF_XDF
> zero-copy sockets.
>
> I'm sending as an RFC because I'm not entirely sure what the desired
> behaviour is here.  Is it Okay to install AF_XDP on queues which don't
> exist?  I presume not?

Your presumption is correct. The idea with the
real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail
out if the queue doesn't exist at bind call. Note that we *didn't* add
any code to avoid the bound queue from being removed via set channel
(your patch 6). Our idea was that if you remove a queue, the ingress
frames would simply stop flowing, and the queue config change checking
was "out-of-band".

I think I prefer your approach, i.e. not allowing the channels/queues
to change if they're bound to an AF_XDP socket. However, your
xdp_umem_query used in ethtool only works for ZC enabled drivers, not
for the existing non-ZC/copy case. If we'd like to go the route of
disabling ethtool_set_channels for an AF_XDP enabled queue this
functionality needs to move the query into netdev core, so we have a
consistent behavior.

> Are the AF_XDP queue_ids referring to TX queues
> as well as RX queues in case of the driver?  I presume not?

We've had a lot of discussions about this internally. Ideally, we'd
like to give driver implementors the most freedom, and not enforcing a
certain queue scheme for Tx. OTOH it makes it weird for the userland
application *not* to have the same id, e.g. if a userland application
would like to get stats or configure the AF_XDP bound Tx queue --
which id is it? Should the Tx queue id  for an xsk be exposed in
sysfs? If the id is *not* the same, would it be OK to change the
number of channels and Tx would continue to operate correctly? A
related question; An xsk with *only* Tx, should it be constrained by
the number of (enabled) Rx queues?

I'd be happy to hear some more opinions/thoughts on this...

> Should
> we try to prevent disabling queues which have non zero-copy sockets
> installed as well? :S
>

Yes, the ZC/non-ZC case must be consistent IMO. See comment above.

> Anyway, if any of those patches seem useful and reasonable, please let
> me know I will repost as non-RFC.
>

I definitely think patch 2 and 3 (and probably 1) should go as non-RFC!

Thanks for spotting that we're not holding the rtnl lock when checking
the # queues (patch 4)!


Björn

> Jakub Kicinski (6):
>   net: update real_num_rx_queues even when !CONFIG_SYSFS
>   xsk: refactor xdp_umem_assign_dev()
>   xsk: don't allow umem replace at stack level
>   xsk: don't allow installing UMEM beyond the number of queues
>   ethtool: rename local variable max -> curr
>   ethtool: don't allow disabling queues with umem installed
>
>  include/linux/netdevice.h | 16 +++++++--
>  net/core/ethtool.c        | 19 ++++++----
>  net/xdp/xdp_umem.c        | 73 ++++++++++++++++++++++++---------------
>  3 files changed, 71 insertions(+), 37 deletions(-)
>
> --
> 2.17.1
>

Re: [RFC bpf-next 0/6] net: xsk: minor improvements around queue handling

[Jakub Kicinski]

On Mon, 30 Jul 2018 14:49:32 +0200, Björn Töpel wrote:
> Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski:
> >
> > Hi!
> 
> Thanks for spending your time on this, Jakub. I'm (temporarily) back
> for a week, so you can expect faster replies now...
> 
> > This set tries to make the core take care of error checking for the
> > drivers.  In particular making sure that the AF_XDP UMEM is not installed
> > on queues which don't exist (or are disabled) and that changing queue
> > (AKA ethtool channel) count cannot disable queues with active AF_XDF
> > zero-copy sockets.
> >
> > I'm sending as an RFC because I'm not entirely sure what the desired
> > behaviour is here.  Is it Okay to install AF_XDP on queues which don't
> > exist?  I presume not?  
> 
> Your presumption is correct. The idea with the
> real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail
> out if the queue doesn't exist at bind call. Note that we *didn't* add
> any code to avoid the bound queue from being removed via set channel
> (your patch 6). Our idea was that if you remove a queue, the ingress
> frames would simply stop flowing, and the queue config change checking
> was "out-of-band".
> 
> I think I prefer your approach, i.e. not allowing the channels/queues
> to change if they're bound to an AF_XDP socket. However, your
> xdp_umem_query used in ethtool only works for ZC enabled drivers, not
> for the existing non-ZC/copy case. If we'd like to go the route of
> disabling ethtool_set_channels for an AF_XDP enabled queue this
> functionality needs to move the query into netdev core, so we have a
> consistent behavior.

Agreed.  There seems to be no notification for changing the number of
queues and therefore no very clean way to solve this today.  The last
two patches are more of a courtesy to the drivers, to simplify the
data structure for holding the UMEMs.

I could argue that driver and stack are not really apples to apples.
Much like Generic XDP, the skb-based AF_XDP is basically a development
tool and last-resort fallback.  For TX driver will most likely allocate
separate queues, while skb-based will use the stack's queues.  These
are actually different queues.  Stack will also fallback to other queue
in __netdev_pick_tx() if number of queues changes.

But yes, preferably skb-based and ZC should behave the same..  

> > Are the AF_XDP queue_ids referring to TX queues
> > as well as RX queues in case of the driver?  I presume not?  
> 
> We've had a lot of discussions about this internally. Ideally, we'd
> like to give driver implementors the most freedom, and not enforcing a
> certain queue scheme for Tx. 

You say freedom I hear diverging implementations and per-driver
checks in user space ;-)

Practically speaking unless you take the xmit lock there is little
chance of reusing stack's TX queues, so you'd have to allocate a
separate queue one way or the other..  At which point the number of
stack's TX queues has no bearing on AF_XDP ZC.

> OTOH it makes it weird for the userland
> application *not* to have the same id, e.g. if a userland application
> would like to get stats or configure the AF_XDP bound Tx queue --
> which id is it? Should the Tx queue id  for an xsk be exposed in
> sysfs?

I'd not go there.

> If the id is *not* the same, would it be OK to change the number of
> channels and Tx would continue to operate correctly? A related
> question; An xsk with *only* Tx, should it be constrained by the
> number of (enabled) Rx queues?

Good question, are drivers even supposed to care about tx-only/rx-only?
From driver's perspective rx-only socket will simply never have
anything to transmit, and tx-only socket will never successfully
xdp_do_redirect().  So IMHO - yes, tx-only XSK still only cares about
RX queues, driver doesn't know no RX will ever happen.  The fact that
user has to populate the FILL queue on a tx-only socket may be counter
intuitive to many...

I was considering proposing a change to drop the *x-only option in the
net tree, I don't really see much use for it :S  And it potentially
creates weird corner cases.

> I'd be happy to hear some more opinions/thoughts on this...

+1

> > Should we try to prevent disabling queues which have non zero-copy
> > sockets installed as well? :S
> >  
> 
> Yes, the ZC/non-ZC case must be consistent IMO. See comment above.
> 
> > Anyway, if any of those patches seem useful and reasonable, please
> > let me know I will repost as non-RFC.
> >  
> 
> I definitely think patch 2 and 3 (and probably 1) should go as
> non-RFC!

Thanks Björn, I will submit the first three!

> Thanks for spotting that we're not holding the rtnl lock when checking
> the # queues (patch 4)!
> 
> Björn

Re: [RFC bpf-next 0/6] net: xsk: minor improvements around queue handling

[Björn Töpel]

Den tis 31 juli 2018 kl 04:49 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> On Mon, 30 Jul 2018 14:49:32 +0200, Björn Töpel wrote:
> > Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski:
> > >
> > > Hi!
> >
> > Thanks for spending your time on this, Jakub. I'm (temporarily) back
> > for a week, so you can expect faster replies now...
> >
> > > This set tries to make the core take care of error checking for the
> > > drivers.  In particular making sure that the AF_XDP UMEM is not installed
> > > on queues which don't exist (or are disabled) and that changing queue
> > > (AKA ethtool channel) count cannot disable queues with active AF_XDF
> > > zero-copy sockets.
> > >
> > > I'm sending as an RFC because I'm not entirely sure what the desired
> > > behaviour is here.  Is it Okay to install AF_XDP on queues which don't
> > > exist?  I presume not?
> >
> > Your presumption is correct. The idea with the
> > real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail
> > out if the queue doesn't exist at bind call. Note that we *didn't* add
> > any code to avoid the bound queue from being removed via set channel
> > (your patch 6). Our idea was that if you remove a queue, the ingress
> > frames would simply stop flowing, and the queue config change checking
> > was "out-of-band".
> >
> > I think I prefer your approach, i.e. not allowing the channels/queues
> > to change if they're bound to an AF_XDP socket. However, your
> > xdp_umem_query used in ethtool only works for ZC enabled drivers, not
> > for the existing non-ZC/copy case. If we'd like to go the route of
> > disabling ethtool_set_channels for an AF_XDP enabled queue this
> > functionality needs to move the query into netdev core, so we have a
> > consistent behavior.
>
> Agreed.  There seems to be no notification for changing the number of
> queues and therefore no very clean way to solve this today.

I'm probably lacking some history here; Has there been any past
efforts in making channels/queues a "kernel object"? Would it make
sense to add notifications for queue changes analogous to netdev
changes?

> The last
> two patches are more of a courtesy to the drivers, to simplify the
> data structure for holding the UMEMs.
>
> I could argue that driver and stack are not really apples to apples.
> Much like Generic XDP, the skb-based AF_XDP is basically a development
> tool and last-resort fallback.

Hmm... I partially agree. Let me think about it a bit more.

> For TX driver will most likely allocate
> separate queues, while skb-based will use the stack's queues.  These
> are actually different queues.  Stack will also fallback to other queue
> in __netdev_pick_tx() if number of queues changes.
>

Yup, ideally the driver will use a dedicated queue. We had some
thoughts on hijacking the skb Tx queue, and route the stack egress
packets elsewhere, but it ended up way too messy.

> But yes, preferably skb-based and ZC should behave the same..
>
> > > Are the AF_XDP queue_ids referring to TX queues
> > > as well as RX queues in case of the driver?  I presume not?
> >
> > We've had a lot of discussions about this internally. Ideally, we'd
> > like to give driver implementors the most freedom, and not enforcing a
> > certain queue scheme for Tx.
>
> You say freedom I hear diverging implementations and per-driver
> checks in user space ;-)
>

Yeah. :-) Well, for the i40e ZC implementation, the Tx queue id was
not equal to Rx queue id, so to answer your question: "Correct, the
queue id refer to Rx."

> Practically speaking unless you take the xmit lock there is little
> chance of reusing stack's TX queues, so you'd have to allocate a
> separate queue one way or the other..  At which point the number of
> stack's TX queues has no bearing on AF_XDP ZC.
>

Yup, you're right.

> > OTOH it makes it weird for the userland
> > application *not* to have the same id, e.g. if a userland application
> > would like to get stats or configure the AF_XDP bound Tx queue --
> > which id is it? Should the Tx queue id  for an xsk be exposed in
> > sysfs?
>
> I'd not go there.
>

Honestly, me neither. I need to think more about to expose the Tx
queue pulls/knobs for a control plane.

> > If the id is *not* the same, would it be OK to change the number of
> > channels and Tx would continue to operate correctly? A related
> > question; An xsk with *only* Tx, should it be constrained by the
> > number of (enabled) Rx queues?
>
> Good question, are drivers even supposed to care about tx-only/rx-only?
> From driver's perspective rx-only socket will simply never have
> anything to transmit, and tx-only socket will never successfully
> xdp_do_redirect().  So IMHO - yes, tx-only XSK still only cares about
> RX queues, driver doesn't know no RX will ever happen.  The fact that
> user has to populate the FILL queue on a tx-only socket may be counter
> intuitive to many...
>

Not allowing tx-only/rx-only definitely makes it easier. Many NICs,
however, have non-symmetrical # of Tx/Rx queues. Scenarios where one
would have #txq >> #rxq would then be hard to support. Again this
could be worked around with virtual netdevs, so maybe it makes sense
to keep the socket layer simple.

> I was considering proposing a change to drop the *x-only option in the
> net tree, I don't really see much use for it :S  And it potentially
> creates weird corner cases.
>
> > I'd be happy to hear some more opinions/thoughts on this...
>
> +1
>
> > > Should we try to prevent disabling queues which have non zero-copy
> > > sockets installed as well? :S
> > >
> >
> > Yes, the ZC/non-ZC case must be consistent IMO. See comment above.
> >
> > > Anyway, if any of those patches seem useful and reasonable, please
> > > let me know I will repost as non-RFC.
> > >
> >
> > I definitely think patch 2 and 3 (and probably 1) should go as
> > non-RFC!
>
> Thanks Björn, I will submit the first three!
>

Thanks! I've already taken them for a spin and acked them!


Björn

> > Thanks for spotting that we're not holding the rtnl lock when checking
> > the # queues (patch 4)!
> >
> > Björn