[PATCH net v2 0/8] update xdp_features flag according to NIC re-configuration

[PATCH net v2 0/8] update xdp_features flag according to NIC re-configuration

[Lorenzo Bianconi]

Changes since v1:
- rebase on top of net tree
- remove NETDEV_XDP_ACT_NDO_XMIT_SG support in mlx5e driver
- always enable NETDEV_XDP_ACT_NDO_XMIT support in mlx5e driver

Lorenzo Bianconi (7):
  tools: ynl: fix render-max for flags definition
  tools: ynl: fix get_mask utility routine
  xdp: add xdp_set_features_flag utility routine
  net: thunderx: take into account xdp_features setting tx/rx queues
  net: ena: take into account xdp_features setting tx/rx queues
  veth: take into account device reconfiguration for xdp_features flag
  net/mlx5e: take into account device reconfiguration for xdp_features
    flag

Matteo Croce (1):
  mvpp2: take care of xdp_features when reconfiguring queues

 Documentation/netlink/specs/netdev.yaml       |  1 +
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 15 +++++--
 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  6 ++-
 .../ethernet/cavium/thunder/nicvf_ethtool.c   | 17 +++++---
 .../net/ethernet/cavium/thunder/nicvf_main.c  |  4 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 15 +++++--
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  1 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 10 ++++-
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 37 +++++++++++-----
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  3 ++
 drivers/net/veth.c                            | 42 +++++++++++++++++--
 include/net/xdp.h                             | 11 +++++
 include/uapi/linux/netdev.h                   |  2 +
 net/core/xdp.c                                | 26 ++++++++----
 tools/include/uapi/linux/netdev.h             |  2 +
 tools/net/ynl/lib/nlspec.py                   |  6 +--
 tools/net/ynl/ynl-gen-c.py                    | 11 +++--
 17 files changed, 164 insertions(+), 45 deletions(-)

-- 
2.39.2

[PATCH net v2 1/8] tools: ynl: fix render-max for flags definition

[Lorenzo Bianconi]

Properly manage render-max property for flags definition type
introducing mask value and setting it to (last_element << 1) - 1
instead of adding max value set to last_element + 1

Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/net/ynl/ynl-gen-c.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 1bcc5354d800..d47376f19de7 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -1931,9 +1931,14 @@ def render_uapi(family, cw):
 
             if const.get('render-max', False):
                 cw.nl()
-                max_name = c_upper(name_pfx + 'max')
-                cw.p('__' + max_name + ',')
-                cw.p(max_name + ' = (__' + max_name + ' - 1)')
+                if const['type'] == 'flags':
+                    max_name = c_upper(name_pfx + 'mask')
+                    max_val = f' = {enum.get_mask()},'
+                    cw.p(max_name + max_val)
+                else:
+                    max_name = c_upper(name_pfx + 'max')
+                    cw.p('__' + max_name + ',')
+                    cw.p(max_name + ' = (__' + max_name + ' - 1)')
             cw.block_end(line=';')
             cw.nl()
         elif const['type'] == 'const':
-- 
2.39.2

[PATCH net v2 2/8] tools: ynl: fix get_mask utility routine

[Lorenzo Bianconi]

Fix get_mask utility routine in order to take into account possible gaps
in the elements list.

Fixes: be5bea1cc0bf ("net: add basic C code generators for Netlink")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/net/ynl/lib/nlspec.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index a34d088f6743..960a356e8225 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -138,10 +138,8 @@ class SpecEnumSet(SpecElement):
 
     def get_mask(self):
         mask = 0
-        idx = self.yaml.get('value-start', 0)
-        for _ in self.entries.values():
-            mask |= 1 << idx
-            idx += 1
+        for e in self.entries.values():
+            mask += e.user_value()
         return mask
 
 
-- 
2.39.2

[PATCH net v2 3/8] xdp: add xdp_set_features_flag utility routine

[Lorenzo Bianconi]

Introduce xdp_set_features_flag utility routine in order to update
dynamically xdp_features according to the dynamic hw configuration via
ethtool (e.g. changing number of hw rx/tx queues).
Add xdp_clear_features_flag() in order to clear all xdp_feature flag.

Reviewed-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 Documentation/netlink/specs/netdev.yaml |  1 +
 include/net/xdp.h                       | 11 +++++++++++
 include/uapi/linux/netdev.h             |  2 ++
 net/core/xdp.c                          | 26 ++++++++++++++++++-------
 tools/include/uapi/linux/netdev.h       |  2 ++
 5 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index 24de747b5344..753e5914a8b7 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -9,6 +9,7 @@ definitions:
   -
     type: flags
     name: xdp-act
+    render-max: true
     entries:
       -
         name: basic
diff --git a/include/net/xdp.h b/include/net/xdp.h
index d517bfac937b..41c57b8b1671 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -428,12 +428,18 @@ MAX_XDP_METADATA_KFUNC,
 #ifdef CONFIG_NET
 u32 bpf_xdp_metadata_kfunc_id(int id);
 bool bpf_dev_bound_kfunc_id(u32 btf_id);
+void xdp_set_features_flag(struct net_device *dev, xdp_features_t val);
 void xdp_features_set_redirect_target(struct net_device *dev, bool support_sg);
 void xdp_features_clear_redirect_target(struct net_device *dev);
 #else
 static inline u32 bpf_xdp_metadata_kfunc_id(int id) { return 0; }
 static inline bool bpf_dev_bound_kfunc_id(u32 btf_id) { return false; }
 
+static inline void
+xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
+{
+}
+
 static inline void
 xdp_features_set_redirect_target(struct net_device *dev, bool support_sg)
 {
@@ -445,4 +451,9 @@ xdp_features_clear_redirect_target(struct net_device *dev)
 }
 #endif
 
+static inline void xdp_clear_features_flag(struct net_device *dev)
+{
+	xdp_set_features_flag(dev, 0);
+}
+
 #endif /* __LINUX_NET_XDP_H__ */
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index 8c4e3e536c04..ed134fbdfd32 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -33,6 +33,8 @@ enum netdev_xdp_act {
 	NETDEV_XDP_ACT_HW_OFFLOAD = 16,
 	NETDEV_XDP_ACT_RX_SG = 32,
 	NETDEV_XDP_ACT_NDO_XMIT_SG = 64,
+
+	NETDEV_XDP_ACT_MASK = 127,
 };
 
 enum {
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 8c92fc553317..87e654b7d06c 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -774,20 +774,32 @@ static int __init xdp_metadata_init(void)
 }
 late_initcall(xdp_metadata_init);
 
-void xdp_features_set_redirect_target(struct net_device *dev, bool support_sg)
+void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
 {
-	dev->xdp_features |= NETDEV_XDP_ACT_NDO_XMIT;
-	if (support_sg)
-		dev->xdp_features |= NETDEV_XDP_ACT_NDO_XMIT_SG;
+	val &= NETDEV_XDP_ACT_MASK;
+	if (dev->xdp_features == val)
+		return;
 
+	dev->xdp_features = val;
 	call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
 }
+EXPORT_SYMBOL_GPL(xdp_set_features_flag);
+
+void xdp_features_set_redirect_target(struct net_device *dev, bool support_sg)
+{
+	xdp_features_t val = (dev->xdp_features | NETDEV_XDP_ACT_NDO_XMIT);
+
+	if (support_sg)
+		val |= NETDEV_XDP_ACT_NDO_XMIT_SG;
+	xdp_set_features_flag(dev, val);
+}
 EXPORT_SYMBOL_GPL(xdp_features_set_redirect_target);
 
 void xdp_features_clear_redirect_target(struct net_device *dev)
 {
-	dev->xdp_features &= ~(NETDEV_XDP_ACT_NDO_XMIT |
-			       NETDEV_XDP_ACT_NDO_XMIT_SG);
-	call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
+	xdp_features_t val = dev->xdp_features;
+
+	val &= ~(NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_NDO_XMIT_SG);
+	xdp_set_features_flag(dev, val);
 }
 EXPORT_SYMBOL_GPL(xdp_features_clear_redirect_target);
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 8c4e3e536c04..ed134fbdfd32 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -33,6 +33,8 @@ enum netdev_xdp_act {
 	NETDEV_XDP_ACT_HW_OFFLOAD = 16,
 	NETDEV_XDP_ACT_RX_SG = 32,
 	NETDEV_XDP_ACT_NDO_XMIT_SG = 64,
+
+	NETDEV_XDP_ACT_MASK = 127,
 };
 
 enum {
-- 
2.39.2

[PATCH net v2 4/8] net: thunderx: take into account xdp_features setting tx/rx queues

[Lorenzo Bianconi]

thunderx nic allows xdp just if enough hw queues are available for XDP.
Take into account queues configuration setting xdp_features.

Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../net/ethernet/cavium/thunder/nicvf_ethtool.c | 17 +++++++++++------
 .../net/ethernet/cavium/thunder/nicvf_main.c    |  4 +++-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
index e5c71f907852..d8d71bf97983 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
@@ -735,12 +735,17 @@ static int nicvf_set_channels(struct net_device *dev,
 	if (channel->tx_count > nic->max_queues)
 		return -EINVAL;
 
-	if (nic->xdp_prog &&
-	    ((channel->tx_count + channel->rx_count) > nic->max_queues)) {
-		netdev_err(nic->netdev,
-			   "XDP mode, RXQs + TXQs > Max %d\n",
-			   nic->max_queues);
-		return -EINVAL;
+	if (channel->tx_count + channel->rx_count > nic->max_queues) {
+		if (nic->xdp_prog) {
+			netdev_err(nic->netdev,
+				   "XDP mode, RXQs + TXQs > Max %d\n",
+				   nic->max_queues);
+			return -EINVAL;
+		}
+
+		xdp_clear_features_flag(nic->netdev);
+	} else if (!pass1_silicon(nic->pdev)) {
+		xdp_set_features_flag(dev, NETDEV_XDP_ACT_BASIC);
 	}
 
 	if (if_up)
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 8b25313c7f6b..eff350e0bc2a 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -2218,7 +2218,9 @@ static int nicvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netdev->netdev_ops = &nicvf_netdev_ops;
 	netdev->watchdog_timeo = NICVF_TX_TIMEOUT;
 
-	netdev->xdp_features = NETDEV_XDP_ACT_BASIC;
+	if (!pass1_silicon(nic->pdev) &&
+	    nic->rx_queues + nic->tx_queues <= nic->max_queues)
+		netdev->xdp_features = NETDEV_XDP_ACT_BASIC;
 
 	/* MTU range: 64 - 9200 */
 	netdev->min_mtu = NIC_HW_MIN_FRS;
-- 
2.39.2

[PATCH net v2 5/8] net: ena: take into account xdp_features setting tx/rx queues

[Lorenzo Bianconi]

ena nic allows xdp just if enough hw queues are available for XDP.
Take into account queues configuration setting xdp_features.

Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
Reviewed-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/amazon/ena/ena_ethtool.c | 15 ++++++++++++---
 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  6 ++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 8da79eedc057..1d4f2f4d10f2 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -850,11 +850,20 @@ static int ena_set_channels(struct net_device *netdev,
 	struct ena_adapter *adapter = netdev_priv(netdev);
 	u32 count = channels->combined_count;
 	/* The check for max value is already done in ethtool */
-	if (count < ENA_MIN_NUM_IO_QUEUES ||
-	    (ena_xdp_present(adapter) &&
-	    !ena_xdp_legal_queue_count(adapter, count)))
+	if (count < ENA_MIN_NUM_IO_QUEUES)
 		return -EINVAL;
 
+	if (!ena_xdp_legal_queue_count(adapter, count)) {
+		if (ena_xdp_present(adapter))
+			return -EINVAL;
+
+		xdp_clear_features_flag(netdev);
+	} else {
+		xdp_set_features_flag(netdev,
+				      NETDEV_XDP_ACT_BASIC |
+				      NETDEV_XDP_ACT_REDIRECT);
+	}
+
 	return ena_update_queue_count(adapter, count);
 }
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index d3999db7c6a2..cbfe7f977270 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -4105,8 +4105,6 @@ static void ena_set_conf_feat_params(struct ena_adapter *adapter,
 	/* Set offload features */
 	ena_set_dev_offloads(feat, netdev);
 
-	netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT;
-
 	adapter->max_mtu = feat->dev_attr.max_mtu;
 	netdev->max_mtu = adapter->max_mtu;
 	netdev->min_mtu = ENA_MIN_MTU;
@@ -4393,6 +4391,10 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ena_config_debug_area(adapter);
 
+	if (ena_xdp_legal_queue_count(adapter, adapter->num_io_queues))
+		netdev->xdp_features = NETDEV_XDP_ACT_BASIC |
+				       NETDEV_XDP_ACT_REDIRECT;
+
 	memcpy(adapter->netdev->perm_addr, adapter->mac_addr, netdev->addr_len);
 
 	netif_carrier_off(netdev);
-- 
2.39.2

[PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Lorenzo Bianconi]

Take into account tx/rx queues reconfiguration setting device
xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
ndo_xdp_xmit callback.

Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/veth.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 1bb54de7124d..293dc3b2c84a 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1257,6 +1257,26 @@ static int veth_enable_range_safe(struct net_device *dev, int start, int end)
 	return 0;
 }
 
+static void veth_set_xdp_features(struct net_device *dev)
+{
+	struct veth_priv *priv = netdev_priv(dev);
+	struct net_device *peer;
+
+	peer = rcu_dereference(priv->peer);
+	if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
+		xdp_features_t val = NETDEV_XDP_ACT_BASIC |
+				     NETDEV_XDP_ACT_REDIRECT |
+				     NETDEV_XDP_ACT_RX_SG;
+
+		if (priv->_xdp_prog || veth_gro_requested(dev))
+			val |= NETDEV_XDP_ACT_NDO_XMIT |
+			       NETDEV_XDP_ACT_NDO_XMIT_SG;
+		xdp_set_features_flag(dev, val);
+	} else {
+		xdp_clear_features_flag(dev);
+	}
+}
+
 static int veth_set_channels(struct net_device *dev,
 			     struct ethtool_channels *ch)
 {
@@ -1323,6 +1343,12 @@ static int veth_set_channels(struct net_device *dev,
 		if (peer)
 			netif_carrier_on(peer);
 	}
+
+	/* update XDP supported features */
+	veth_set_xdp_features(dev);
+	if (peer)
+		veth_set_xdp_features(peer);
+
 	return err;
 
 revert:
@@ -1489,7 +1515,10 @@ static int veth_set_features(struct net_device *dev,
 		err = veth_napi_enable(dev);
 		if (err)
 			return err;
+
+		xdp_features_set_redirect_target(dev, true);
 	} else {
+		xdp_features_clear_redirect_target(dev);
 		veth_napi_del(dev);
 	}
 	return 0;
@@ -1570,10 +1599,15 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			peer->hw_features &= ~NETIF_F_GSO_SOFTWARE;
 			peer->max_mtu = max_mtu;
 		}
+
+		xdp_features_set_redirect_target(dev, true);
 	}
 
 	if (old_prog) {
 		if (!prog) {
+			if (!veth_gro_requested(dev))
+				xdp_features_clear_redirect_target(dev);
+
 			if (dev->flags & IFF_UP)
 				veth_disable_xdp(dev);
 
@@ -1686,10 +1720,6 @@ static void veth_setup(struct net_device *dev)
 	dev->hw_enc_features = VETH_FEATURES;
 	dev->mpls_features = NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE;
 	netif_set_tso_max_size(dev, GSO_MAX_SIZE);
-
-	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
-			    NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG |
-			    NETDEV_XDP_ACT_NDO_XMIT_SG;
 }
 
 /*
@@ -1857,6 +1887,10 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 		goto err_queues;
 
 	veth_disable_gro(dev);
+	/* update XDP supported features */
+	veth_set_xdp_features(dev);
+	veth_set_xdp_features(peer);
+
 	return 0;
 
 err_queues:
-- 
2.39.2

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Matthieu Baerts]

Hi Lorenzo,

On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> Take into account tx/rx queues reconfiguration setting device
> xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> ndo_xdp_xmit callback.
> 
> Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Thank you for the modification.

Unfortunately, 'git bisect' just told me this modification is the origin
of a new WARN when using veth in a netns:


###################### 8< ######################

=============================
WARNING: suspicious RCU usage
6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
-----------------------------
drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
1 lock held by ip/135:
#0: ffffffff8dc4b108 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg
(net/core/rtnetlink.c:6172)

stack backtrace:
CPU: 1 PID: 135 Comm: ip Not tainted 6.3.0-rc1-00144-g064d70527aaa #149
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1
04/01/2014
Call Trace:
 <TASK>
dump_stack_lvl (lib/dump_stack.c:107)
lockdep_rcu_suspicious (include/linux/context_tracking.h:152)
veth_set_xdp_features (drivers/net/veth.c:1265 (discriminator 9))
veth_newlink (drivers/net/veth.c:1892)
? veth_set_features (drivers/net/veth.c:1774)
? kasan_save_stack (mm/kasan/common.c:47)
? kasan_save_stack (mm/kasan/common.c:46)
? kasan_set_track (mm/kasan/common.c:52)
? alloc_netdev_mqs (include/linux/slab.h:737)
? rcu_read_lock_sched_held (kernel/rcu/update.c:125)
? trace_kmalloc (include/trace/events/kmem.h:54)
? __xdp_rxq_info_reg (net/core/xdp.c:188)
? alloc_netdev_mqs (net/core/dev.c:10657)
? rtnl_create_link (net/core/rtnetlink.c:3312)
rtnl_newlink_create (net/core/rtnetlink.c:3440)
? rtnl_link_get_net_capable.constprop.0 (net/core/rtnetlink.c:3391)
__rtnl_newlink (net/core/rtnetlink.c:3657)
? lock_downgrade (kernel/locking/lockdep.c:5321)
? rtnl_link_unregister (net/core/rtnetlink.c:3487)
rtnl_newlink (net/core/rtnetlink.c:3671)
rtnetlink_rcv_msg (net/core/rtnetlink.c:6174)
? rtnl_link_fill (net/core/rtnetlink.c:6070)
? mark_usage (kernel/locking/lockdep.c:4914)
? mark_usage (kernel/locking/lockdep.c:4914)
netlink_rcv_skb (net/netlink/af_netlink.c:2574)
? rtnl_link_fill (net/core/rtnetlink.c:6070)
? netlink_ack (net/netlink/af_netlink.c:2551)
? lock_acquire (kernel/locking/lockdep.c:467)
? net_generic (include/linux/rcupdate.h:805)
? netlink_deliver_tap (include/linux/rcupdate.h:805)
netlink_unicast (net/netlink/af_netlink.c:1340)
? netlink_attachskb (net/netlink/af_netlink.c:1350)
netlink_sendmsg (net/netlink/af_netlink.c:1942)
? netlink_unicast (net/netlink/af_netlink.c:1861)
? netlink_unicast (net/netlink/af_netlink.c:1861)
sock_sendmsg (net/socket.c:727)
____sys_sendmsg (net/socket.c:2501)
? kernel_sendmsg (net/socket.c:2448)
? __copy_msghdr (net/socket.c:2428)
___sys_sendmsg (net/socket.c:2557)
? mark_usage (kernel/locking/lockdep.c:4914)
? do_recvmmsg (net/socket.c:2544)
? lock_acquire (kernel/locking/lockdep.c:467)
? find_held_lock (kernel/locking/lockdep.c:5159)
? __lock_release (kernel/locking/lockdep.c:5345)
? __might_fault (mm/memory.c:5625)
? lock_downgrade (kernel/locking/lockdep.c:5321)
? __fget_light (include/linux/atomic/atomic-arch-fallback.h:227)
__sys_sendmsg (include/linux/file.h:31)
? __sys_sendmsg_sock (net/socket.c:2572)
? rseq_get_rseq_cs (kernel/rseq.c:275)
? lockdep_hardirqs_on_prepare.part.0 (kernel/locking/lockdep.c:4263)
do_syscall_64 (arch/x86/entry/common.c:50)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
RIP: 0033:0x7f0d1aadeb17
Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e
fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00
f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
All code
========
   0:   0f 00                   (bad)
   2:   f7 d8                   neg    %eax
   4:   64 89 02                mov    %eax,%fs:(%rdx)
   7:   48 c7 c0 ff ff ff ff    mov    $0xffffffffffffffff,%rax
   e:   eb b9                   jmp    0xffffffffffffffc9
  10:   0f 1f 00                nopl   (%rax)
  13:   f3 0f 1e fa             endbr64
  17:   64 8b 04 25 18 00 00    mov    %fs:0x18,%eax
  1e:   00
  1f:   85 c0                   test   %eax,%eax
  21:   75 10                   jne    0x33
  23:   b8 2e 00 00 00          mov    $0x2e,%eax
  28:   0f 05                   syscall
  2a:*  48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
<-- trapping instruction
  30:   77 51                   ja     0x83
  32:   c3                      ret
  33:   48 83 ec 28             sub    $0x28,%rsp
  37:   89 54 24 1c             mov    %edx,0x1c(%rsp)
  3b:   48 89 74 24 10          mov    %rsi,0x10(%rsp)

Code starting with the faulting instruction
===========================================
   0:   48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
   6:   77 51                   ja     0x59
   8:   c3                      ret
   9:   48 83 ec 28             sub    $0x28,%rsp
   d:   89 54 24 1c             mov    %edx,0x1c(%rsp)
  11:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
RSP: 002b:00007ffca3305d48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000640f2bb2 RCX: 00007f0d1aadeb17
RDX: 0000000000000000 RSI: 00007ffca3305db0 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000001 R09: 00007ffca3304ae0
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffca3305eb4 R14: 00007ffca3305e80 R15: 0000561e28bf5020
 </TASK>
ip (135) used greatest stack depth: 24544 bytes left

###################### 8< ######################


I can reproduce the issue on the "net" tree with just these 3 commands:

# ip netns add ns1
# ip netns add ns2
# ip link add ns1eth1 netns ns1 type veth peer name ns2eth1 netns ns2

Without this commit fccca038f300 ("veth: take into account device
reconfiguration for xdp_features flag"), I don't have the issue.

For more details about the issue detected by CIs validating our MPTCP
tree, including kconfig and vmlinux if needed:

  https://github.com/multipath-tcp/mptcp_net-next/issues/372


Do you mind looking at this regression please? :)


On our side, we will revert this patch in our tree for the moment to
unblock our CI jobs.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Eric Dumazet]

On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Lorenzo,
>
> On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> > Take into account tx/rx queues reconfiguration setting device
> > xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> > ndo_xdp_xmit callback.
> >
> > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> Thank you for the modification.
>
> Unfortunately, 'git bisect' just told me this modification is the origin
> of a new WARN when using veth in a netns:
>
>
> ###################### 8< ######################
>
> =============================
> WARNING: suspicious RCU usage
> 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
> -----------------------------
> drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
>

Same observation here, I am releasing a syzbot report with a repro.



>
> rcu_scheduler_active = 2, debug_locks = 1
> 1 lock held by ip/135:
> #0: ffffffff8dc4b108 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg
> (net/core/rtnetlink.c:6172)
>
> stack backtrace:
> CPU: 1 PID: 135 Comm: ip Not tainted 6.3.0-rc1-00144-g064d70527aaa #149
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1
> 04/01/2014
> Call Trace:
>  <TASK>
> dump_stack_lvl (lib/dump_stack.c:107)
> lockdep_rcu_suspicious (include/linux/context_tracking.h:152)
> veth_set_xdp_features (drivers/net/veth.c:1265 (discriminator 9))
> veth_newlink (drivers/net/veth.c:1892)
> ? veth_set_features (drivers/net/veth.c:1774)
> ? kasan_save_stack (mm/kasan/common.c:47)
> ? kasan_save_stack (mm/kasan/common.c:46)
> ? kasan_set_track (mm/kasan/common.c:52)
> ? alloc_netdev_mqs (include/linux/slab.h:737)
> ? rcu_read_lock_sched_held (kernel/rcu/update.c:125)
> ? trace_kmalloc (include/trace/events/kmem.h:54)
> ? __xdp_rxq_info_reg (net/core/xdp.c:188)
> ? alloc_netdev_mqs (net/core/dev.c:10657)
> ? rtnl_create_link (net/core/rtnetlink.c:3312)
> rtnl_newlink_create (net/core/rtnetlink.c:3440)
> ? rtnl_link_get_net_capable.constprop.0 (net/core/rtnetlink.c:3391)
> __rtnl_newlink (net/core/rtnetlink.c:3657)
> ? lock_downgrade (kernel/locking/lockdep.c:5321)
> ? rtnl_link_unregister (net/core/rtnetlink.c:3487)
> rtnl_newlink (net/core/rtnetlink.c:3671)
> rtnetlink_rcv_msg (net/core/rtnetlink.c:6174)
> ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> ? mark_usage (kernel/locking/lockdep.c:4914)
> ? mark_usage (kernel/locking/lockdep.c:4914)
> netlink_rcv_skb (net/netlink/af_netlink.c:2574)
> ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> ? netlink_ack (net/netlink/af_netlink.c:2551)
> ? lock_acquire (kernel/locking/lockdep.c:467)
> ? net_generic (include/linux/rcupdate.h:805)
> ? netlink_deliver_tap (include/linux/rcupdate.h:805)
> netlink_unicast (net/netlink/af_netlink.c:1340)
> ? netlink_attachskb (net/netlink/af_netlink.c:1350)
> netlink_sendmsg (net/netlink/af_netlink.c:1942)
> ? netlink_unicast (net/netlink/af_netlink.c:1861)
> ? netlink_unicast (net/netlink/af_netlink.c:1861)
> sock_sendmsg (net/socket.c:727)
> ____sys_sendmsg (net/socket.c:2501)
> ? kernel_sendmsg (net/socket.c:2448)
> ? __copy_msghdr (net/socket.c:2428)
> ___sys_sendmsg (net/socket.c:2557)
> ? mark_usage (kernel/locking/lockdep.c:4914)
> ? do_recvmmsg (net/socket.c:2544)
> ? lock_acquire (kernel/locking/lockdep.c:467)
> ? find_held_lock (kernel/locking/lockdep.c:5159)
> ? __lock_release (kernel/locking/lockdep.c:5345)
> ? __might_fault (mm/memory.c:5625)
> ? lock_downgrade (kernel/locking/lockdep.c:5321)
> ? __fget_light (include/linux/atomic/atomic-arch-fallback.h:227)
> __sys_sendmsg (include/linux/file.h:31)
> ? __sys_sendmsg_sock (net/socket.c:2572)
> ? rseq_get_rseq_cs (kernel/rseq.c:275)
> ? lockdep_hardirqs_on_prepare.part.0 (kernel/locking/lockdep.c:4263)
> do_syscall_64 (arch/x86/entry/common.c:50)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> RIP: 0033:0x7f0d1aadeb17
> Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e
> fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00
> f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
> All code
> ========
>    0:   0f 00                   (bad)
>    2:   f7 d8                   neg    %eax
>    4:   64 89 02                mov    %eax,%fs:(%rdx)
>    7:   48 c7 c0 ff ff ff ff    mov    $0xffffffffffffffff,%rax
>    e:   eb b9                   jmp    0xffffffffffffffc9
>   10:   0f 1f 00                nopl   (%rax)
>   13:   f3 0f 1e fa             endbr64
>   17:   64 8b 04 25 18 00 00    mov    %fs:0x18,%eax
>   1e:   00
>   1f:   85 c0                   test   %eax,%eax
>   21:   75 10                   jne    0x33
>   23:   b8 2e 00 00 00          mov    $0x2e,%eax
>   28:   0f 05                   syscall
>   2a:*  48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
> <-- trapping instruction
>   30:   77 51                   ja     0x83
>   32:   c3                      ret
>   33:   48 83 ec 28             sub    $0x28,%rsp
>   37:   89 54 24 1c             mov    %edx,0x1c(%rsp)
>   3b:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
>
> Code starting with the faulting instruction
> ===========================================
>    0:   48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
>    6:   77 51                   ja     0x59
>    8:   c3                      ret
>    9:   48 83 ec 28             sub    $0x28,%rsp
>    d:   89 54 24 1c             mov    %edx,0x1c(%rsp)
>   11:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
> RSP: 002b:00007ffca3305d48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00000000640f2bb2 RCX: 00007f0d1aadeb17
> RDX: 0000000000000000 RSI: 00007ffca3305db0 RDI: 0000000000000003
> RBP: 0000000000000000 R08: 0000000000000001 R09: 00007ffca3304ae0
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> R13: 00007ffca3305eb4 R14: 00007ffca3305e80 R15: 0000561e28bf5020
>  </TASK>
> ip (135) used greatest stack depth: 24544 bytes left
>
> ###################### 8< ######################
>
>
> I can reproduce the issue on the "net" tree with just these 3 commands:
>
> # ip netns add ns1
> # ip netns add ns2
> # ip link add ns1eth1 netns ns1 type veth peer name ns2eth1 netns ns2
>
> Without this commit fccca038f300 ("veth: take into account device
> reconfiguration for xdp_features flag"), I don't have the issue.
>
> For more details about the issue detected by CIs validating our MPTCP
> tree, including kconfig and vmlinux if needed:
>
>   https://github.com/multipath-tcp/mptcp_net-next/issues/372
>
>
> Do you mind looking at this regression please? :)
>
>
> On our side, we will revert this patch in our tree for the moment to
> unblock our CI jobs.
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Eric Dumazet]

On Mon, Mar 13, 2023 at 8:50 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
> >
> > Hi Lorenzo,
> >
> > On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> > > Take into account tx/rx queues reconfiguration setting device
> > > xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> > > ndo_xdp_xmit callback.
> > >
> > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >
> > Thank you for the modification.
> >
> > Unfortunately, 'git bisect' just told me this modification is the origin
> > of a new WARN when using veth in a netns:
> >
> >
> > ###################### 8< ######################
> >
> > =============================
> > WARNING: suspicious RCU usage
> > 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
> > -----------------------------
> > drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
> >
> > other info that might help us debug this:
> >
>
> Same observation here, I am releasing a syzbot report with a repro.
>
>

I guess a fix would be:

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 293dc3b2c84a6c1931e8df42cdcd5f2798004f3c..4da74ac27f9a2425d8d3f4ffcc93f453bd58e3a5
100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1262,7 +1262,7 @@ static void veth_set_xdp_features(struct net_device *dev)
        struct veth_priv *priv = netdev_priv(dev);
        struct net_device *peer;

-       peer = rcu_dereference(priv->peer);
+       peer = rtnl_dereference(priv->peer);
        if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
                xdp_features_t val = NETDEV_XDP_ACT_BASIC |
                                     NETDEV_XDP_ACT_REDIRECT |


>
> >
> > rcu_scheduler_active = 2, debug_locks = 1
> > 1 lock held by ip/135:
> > #0: ffffffff8dc4b108 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg
> > (net/core/rtnetlink.c:6172)
> >
> > stack backtrace:
> > CPU: 1 PID: 135 Comm: ip Not tainted 6.3.0-rc1-00144-g064d70527aaa #149
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1
> > 04/01/2014
> > Call Trace:
> >  <TASK>
> > dump_stack_lvl (lib/dump_stack.c:107)
> > lockdep_rcu_suspicious (include/linux/context_tracking.h:152)
> > veth_set_xdp_features (drivers/net/veth.c:1265 (discriminator 9))
> > veth_newlink (drivers/net/veth.c:1892)
> > ? veth_set_features (drivers/net/veth.c:1774)
> > ? kasan_save_stack (mm/kasan/common.c:47)
> > ? kasan_save_stack (mm/kasan/common.c:46)
> > ? kasan_set_track (mm/kasan/common.c:52)
> > ? alloc_netdev_mqs (include/linux/slab.h:737)
> > ? rcu_read_lock_sched_held (kernel/rcu/update.c:125)
> > ? trace_kmalloc (include/trace/events/kmem.h:54)
> > ? __xdp_rxq_info_reg (net/core/xdp.c:188)
> > ? alloc_netdev_mqs (net/core/dev.c:10657)
> > ? rtnl_create_link (net/core/rtnetlink.c:3312)
> > rtnl_newlink_create (net/core/rtnetlink.c:3440)
> > ? rtnl_link_get_net_capable.constprop.0 (net/core/rtnetlink.c:3391)
> > __rtnl_newlink (net/core/rtnetlink.c:3657)
> > ? lock_downgrade (kernel/locking/lockdep.c:5321)
> > ? rtnl_link_unregister (net/core/rtnetlink.c:3487)
> > rtnl_newlink (net/core/rtnetlink.c:3671)
> > rtnetlink_rcv_msg (net/core/rtnetlink.c:6174)
> > ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> > ? mark_usage (kernel/locking/lockdep.c:4914)
> > ? mark_usage (kernel/locking/lockdep.c:4914)
> > netlink_rcv_skb (net/netlink/af_netlink.c:2574)
> > ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> > ? netlink_ack (net/netlink/af_netlink.c:2551)
> > ? lock_acquire (kernel/locking/lockdep.c:467)
> > ? net_generic (include/linux/rcupdate.h:805)
> > ? netlink_deliver_tap (include/linux/rcupdate.h:805)
> > netlink_unicast (net/netlink/af_netlink.c:1340)
> > ? netlink_attachskb (net/netlink/af_netlink.c:1350)
> > netlink_sendmsg (net/netlink/af_netlink.c:1942)
> > ? netlink_unicast (net/netlink/af_netlink.c:1861)
> > ? netlink_unicast (net/netlink/af_netlink.c:1861)
> > sock_sendmsg (net/socket.c:727)
> > ____sys_sendmsg (net/socket.c:2501)
> > ? kernel_sendmsg (net/socket.c:2448)
> > ? __copy_msghdr (net/socket.c:2428)
> > ___sys_sendmsg (net/socket.c:2557)
> > ? mark_usage (kernel/locking/lockdep.c:4914)
> > ? do_recvmmsg (net/socket.c:2544)
> > ? lock_acquire (kernel/locking/lockdep.c:467)
> > ? find_held_lock (kernel/locking/lockdep.c:5159)
> > ? __lock_release (kernel/locking/lockdep.c:5345)
> > ? __might_fault (mm/memory.c:5625)
> > ? lock_downgrade (kernel/locking/lockdep.c:5321)
> > ? __fget_light (include/linux/atomic/atomic-arch-fallback.h:227)
> > __sys_sendmsg (include/linux/file.h:31)
> > ? __sys_sendmsg_sock (net/socket.c:2572)
> > ? rseq_get_rseq_cs (kernel/rseq.c:275)
> > ? lockdep_hardirqs_on_prepare.part.0 (kernel/locking/lockdep.c:4263)
> > do_syscall_64 (arch/x86/entry/common.c:50)
> > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> > RIP: 0033:0x7f0d1aadeb17
> > Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e
> > fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00
> > f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
> > All code
> > ========
> >    0:   0f 00                   (bad)
> >    2:   f7 d8                   neg    %eax
> >    4:   64 89 02                mov    %eax,%fs:(%rdx)
> >    7:   48 c7 c0 ff ff ff ff    mov    $0xffffffffffffffff,%rax
> >    e:   eb b9                   jmp    0xffffffffffffffc9
> >   10:   0f 1f 00                nopl   (%rax)
> >   13:   f3 0f 1e fa             endbr64
> >   17:   64 8b 04 25 18 00 00    mov    %fs:0x18,%eax
> >   1e:   00
> >   1f:   85 c0                   test   %eax,%eax
> >   21:   75 10                   jne    0x33
> >   23:   b8 2e 00 00 00          mov    $0x2e,%eax
> >   28:   0f 05                   syscall
> >   2a:*  48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
> > <-- trapping instruction
> >   30:   77 51                   ja     0x83
> >   32:   c3                      ret
> >   33:   48 83 ec 28             sub    $0x28,%rsp
> >   37:   89 54 24 1c             mov    %edx,0x1c(%rsp)
> >   3b:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
> >
> > Code starting with the faulting instruction
> > ===========================================
> >    0:   48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
> >    6:   77 51                   ja     0x59
> >    8:   c3                      ret
> >    9:   48 83 ec 28             sub    $0x28,%rsp
> >    d:   89 54 24 1c             mov    %edx,0x1c(%rsp)
> >   11:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
> > RSP: 002b:00007ffca3305d48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > RAX: ffffffffffffffda RBX: 00000000640f2bb2 RCX: 00007f0d1aadeb17
> > RDX: 0000000000000000 RSI: 00007ffca3305db0 RDI: 0000000000000003
> > RBP: 0000000000000000 R08: 0000000000000001 R09: 00007ffca3304ae0
> > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> > R13: 00007ffca3305eb4 R14: 00007ffca3305e80 R15: 0000561e28bf5020
> >  </TASK>
> > ip (135) used greatest stack depth: 24544 bytes left
> >
> > ###################### 8< ######################
> >
> >
> > I can reproduce the issue on the "net" tree with just these 3 commands:
> >
> > # ip netns add ns1
> > # ip netns add ns2
> > # ip link add ns1eth1 netns ns1 type veth peer name ns2eth1 netns ns2
> >
> > Without this commit fccca038f300 ("veth: take into account device
> > reconfiguration for xdp_features flag"), I don't have the issue.
> >
> > For more details about the issue detected by CIs validating our MPTCP
> > tree, including kconfig and vmlinux if needed:
> >
> >   https://github.com/multipath-tcp/mptcp_net-next/issues/372
> >
> >
> > Do you mind looking at this regression please? :)
> >
> >
> > On our side, we will revert this patch in our tree for the moment to
> > unblock our CI jobs.
> >
> > Cheers,
> > Matt
> > --
> > Tessares | Belgium | Hybrid Access Solutions
> > www.tessares.net

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Matthieu Baerts]

Hi Eric,

On 13/03/2023 16:53, Eric Dumazet wrote:
> On Mon, Mar 13, 2023 at 8:50 AM Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
>> <matthieu.baerts@tessares.net> wrote:
>>>
>>> Hi Lorenzo,
>>>
>>> On 09/03/2023 13:25, Lorenzo Bianconi wrote:
>>>> Take into account tx/rx queues reconfiguration setting device
>>>> xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
>>>> ndo_xdp_xmit callback.
>>>>
>>>> Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>>
>>> Thank you for the modification.
>>>
>>> Unfortunately, 'git bisect' just told me this modification is the origin
>>> of a new WARN when using veth in a netns:
>>>
>>>
>>> ###################### 8< ######################
>>>
>>> =============================
>>> WARNING: suspicious RCU usage
>>> 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
>>> -----------------------------
>>> drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
>>>
>>> other info that might help us debug this:
>>>
>>
>> Same observation here, I am releasing a syzbot report with a repro.
>>
>>
> 
> I guess a fix would be:
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 293dc3b2c84a6c1931e8df42cdcd5f2798004f3c..4da74ac27f9a2425d8d3f4ffcc93f453bd58e3a5
> 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1262,7 +1262,7 @@ static void veth_set_xdp_features(struct net_device *dev)
>         struct veth_priv *priv = netdev_priv(dev);
>         struct net_device *peer;
> 
> -       peer = rcu_dereference(priv->peer);
> +       peer = rtnl_dereference(priv->peer);
>         if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
>                 xdp_features_t val = NETDEV_XDP_ACT_BASIC |
>                                      NETDEV_XDP_ACT_REDIRECT |
> 

Thank you for having looked!

This patch avoids the warning on our side.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8642 bytes --]

> On Mon, Mar 13, 2023 at 8:50 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
> > <matthieu.baerts@tessares.net> wrote:
> > >
> > > Hi Lorenzo,
> > >
> > > On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> > > > Take into account tx/rx queues reconfiguration setting device
> > > > xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> > > > ndo_xdp_xmit callback.
> > > >
> > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > >
> > > Thank you for the modification.
> > >
> > > Unfortunately, 'git bisect' just told me this modification is the origin
> > > of a new WARN when using veth in a netns:
> > >
> > >
> > > ###################### 8< ######################
> > >
> > > =============================
> > > WARNING: suspicious RCU usage
> > > 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
> > > -----------------------------
> > > drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
> > >
> > > other info that might help us debug this:
> > >
> >
> > Same observation here, I am releasing a syzbot report with a repro.
> >
> >
> 
> I guess a fix would be:
> 

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 293dc3b2c84a6c1931e8df42cdcd5f2798004f3c..4da74ac27f9a2425d8d3f4ffcc93f453bd58e3a5
> 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1262,7 +1262,7 @@ static void veth_set_xdp_features(struct net_device *dev)
>         struct veth_priv *priv = netdev_priv(dev);
>         struct net_device *peer;
> 
> -       peer = rcu_dereference(priv->peer);
> +       peer = rtnl_dereference(priv->peer);
>         if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
>                 xdp_features_t val = NETDEV_XDP_ACT_BASIC |
>                                      NETDEV_XDP_ACT_REDIRECT |
> 
> 
> >
> > >
> > > rcu_scheduler_active = 2, debug_locks = 1
> > > 1 lock held by ip/135:
> > > #0: ffffffff8dc4b108 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg
> > > (net/core/rtnetlink.c:6172)
> > >
> > > stack backtrace:
> > > CPU: 1 PID: 135 Comm: ip Not tainted 6.3.0-rc1-00144-g064d70527aaa #149
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1
> > > 04/01/2014
> > > Call Trace:
> > >  <TASK>
> > > dump_stack_lvl (lib/dump_stack.c:107)
> > > lockdep_rcu_suspicious (include/linux/context_tracking.h:152)
> > > veth_set_xdp_features (drivers/net/veth.c:1265 (discriminator 9))
> > > veth_newlink (drivers/net/veth.c:1892)
> > > ? veth_set_features (drivers/net/veth.c:1774)
> > > ? kasan_save_stack (mm/kasan/common.c:47)
> > > ? kasan_save_stack (mm/kasan/common.c:46)
> > > ? kasan_set_track (mm/kasan/common.c:52)
> > > ? alloc_netdev_mqs (include/linux/slab.h:737)
> > > ? rcu_read_lock_sched_held (kernel/rcu/update.c:125)
> > > ? trace_kmalloc (include/trace/events/kmem.h:54)
> > > ? __xdp_rxq_info_reg (net/core/xdp.c:188)
> > > ? alloc_netdev_mqs (net/core/dev.c:10657)
> > > ? rtnl_create_link (net/core/rtnetlink.c:3312)
> > > rtnl_newlink_create (net/core/rtnetlink.c:3440)
> > > ? rtnl_link_get_net_capable.constprop.0 (net/core/rtnetlink.c:3391)
> > > __rtnl_newlink (net/core/rtnetlink.c:3657)
> > > ? lock_downgrade (kernel/locking/lockdep.c:5321)
> > > ? rtnl_link_unregister (net/core/rtnetlink.c:3487)
> > > rtnl_newlink (net/core/rtnetlink.c:3671)
> > > rtnetlink_rcv_msg (net/core/rtnetlink.c:6174)
> > > ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> > > ? mark_usage (kernel/locking/lockdep.c:4914)
> > > ? mark_usage (kernel/locking/lockdep.c:4914)
> > > netlink_rcv_skb (net/netlink/af_netlink.c:2574)
> > > ? rtnl_link_fill (net/core/rtnetlink.c:6070)
> > > ? netlink_ack (net/netlink/af_netlink.c:2551)
> > > ? lock_acquire (kernel/locking/lockdep.c:467)
> > > ? net_generic (include/linux/rcupdate.h:805)
> > > ? netlink_deliver_tap (include/linux/rcupdate.h:805)
> > > netlink_unicast (net/netlink/af_netlink.c:1340)
> > > ? netlink_attachskb (net/netlink/af_netlink.c:1350)
> > > netlink_sendmsg (net/netlink/af_netlink.c:1942)
> > > ? netlink_unicast (net/netlink/af_netlink.c:1861)
> > > ? netlink_unicast (net/netlink/af_netlink.c:1861)
> > > sock_sendmsg (net/socket.c:727)
> > > ____sys_sendmsg (net/socket.c:2501)
> > > ? kernel_sendmsg (net/socket.c:2448)
> > > ? __copy_msghdr (net/socket.c:2428)
> > > ___sys_sendmsg (net/socket.c:2557)
> > > ? mark_usage (kernel/locking/lockdep.c:4914)
> > > ? do_recvmmsg (net/socket.c:2544)
> > > ? lock_acquire (kernel/locking/lockdep.c:467)
> > > ? find_held_lock (kernel/locking/lockdep.c:5159)
> > > ? __lock_release (kernel/locking/lockdep.c:5345)
> > > ? __might_fault (mm/memory.c:5625)
> > > ? lock_downgrade (kernel/locking/lockdep.c:5321)
> > > ? __fget_light (include/linux/atomic/atomic-arch-fallback.h:227)
> > > __sys_sendmsg (include/linux/file.h:31)
> > > ? __sys_sendmsg_sock (net/socket.c:2572)
> > > ? rseq_get_rseq_cs (kernel/rseq.c:275)
> > > ? lockdep_hardirqs_on_prepare.part.0 (kernel/locking/lockdep.c:4263)
> > > do_syscall_64 (arch/x86/entry/common.c:50)
> > > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> > > RIP: 0033:0x7f0d1aadeb17
> > > Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e
> > > fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00
> > > f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
> > > All code
> > > ========
> > >    0:   0f 00                   (bad)
> > >    2:   f7 d8                   neg    %eax
> > >    4:   64 89 02                mov    %eax,%fs:(%rdx)
> > >    7:   48 c7 c0 ff ff ff ff    mov    $0xffffffffffffffff,%rax
> > >    e:   eb b9                   jmp    0xffffffffffffffc9
> > >   10:   0f 1f 00                nopl   (%rax)
> > >   13:   f3 0f 1e fa             endbr64
> > >   17:   64 8b 04 25 18 00 00    mov    %fs:0x18,%eax
> > >   1e:   00
> > >   1f:   85 c0                   test   %eax,%eax
> > >   21:   75 10                   jne    0x33
> > >   23:   b8 2e 00 00 00          mov    $0x2e,%eax
> > >   28:   0f 05                   syscall
> > >   2a:*  48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
> > > <-- trapping instruction
> > >   30:   77 51                   ja     0x83
> > >   32:   c3                      ret
> > >   33:   48 83 ec 28             sub    $0x28,%rsp
> > >   37:   89 54 24 1c             mov    %edx,0x1c(%rsp)
> > >   3b:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
> > >
> > > Code starting with the faulting instruction
> > > ===========================================
> > >    0:   48 3d 00 f0 ff ff       cmp    $0xfffffffffffff000,%rax
> > >    6:   77 51                   ja     0x59
> > >    8:   c3                      ret
> > >    9:   48 83 ec 28             sub    $0x28,%rsp
> > >    d:   89 54 24 1c             mov    %edx,0x1c(%rsp)
> > >   11:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
> > > RSP: 002b:00007ffca3305d48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> > > RAX: ffffffffffffffda RBX: 00000000640f2bb2 RCX: 00007f0d1aadeb17
> > > RDX: 0000000000000000 RSI: 00007ffca3305db0 RDI: 0000000000000003
> > > RBP: 0000000000000000 R08: 0000000000000001 R09: 00007ffca3304ae0
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> > > R13: 00007ffca3305eb4 R14: 00007ffca3305e80 R15: 0000561e28bf5020
> > >  </TASK>
> > > ip (135) used greatest stack depth: 24544 bytes left
> > >
> > > ###################### 8< ######################
> > >
> > >
> > > I can reproduce the issue on the "net" tree with just these 3 commands:
> > >
> > > # ip netns add ns1
> > > # ip netns add ns2
> > > # ip link add ns1eth1 netns ns1 type veth peer name ns2eth1 netns ns2
> > >
> > > Without this commit fccca038f300 ("veth: take into account device
> > > reconfiguration for xdp_features flag"), I don't have the issue.
> > >
> > > For more details about the issue detected by CIs validating our MPTCP
> > > tree, including kconfig and vmlinux if needed:
> > >
> > >   https://github.com/multipath-tcp/mptcp_net-next/issues/372
> > >
> > >
> > > Do you mind looking at this regression please? :)
> > >
> > >
> > > On our side, we will revert this patch in our tree for the moment to
> > > unblock our CI jobs.
> > >
> > > Cheers,
> > > Matt
> > > --
> > > Tessares | Belgium | Hybrid Access Solutions
> > > www.tessares.net

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Eric Dumazet]

On Mon, Mar 13, 2023 at 9:36 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On Mon, Mar 13, 2023 at 8:50 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
> > > <matthieu.baerts@tessares.net> wrote:
> > > >
> > > > Hi Lorenzo,
> > > >
> > > > On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> > > > > Take into account tx/rx queues reconfiguration setting device
> > > > > xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> > > > > ndo_xdp_xmit callback.
> > > > >
> > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > >
> > > > Thank you for the modification.
> > > >
> > > > Unfortunately, 'git bisect' just told me this modification is the origin
> > > > of a new WARN when using veth in a netns:
> > > >
> > > >
> > > > ###################### 8< ######################
> > > >
> > > > =============================
> > > > WARNING: suspicious RCU usage
> > > > 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
> > > > -----------------------------
> > > > drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
> > > >
> > > > other info that might help us debug this:
> > > >
> > >
> > > Same observation here, I am releasing a syzbot report with a repro.
> > >
> > >
> >
> > I guess a fix would be:
> >
>
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

Can you submit a formal fix ?

Thanks.

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

> On Mon, Mar 13, 2023 at 9:36 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > On Mon, Mar 13, 2023 at 8:50 AM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Mon, Mar 13, 2023 at 7:15 AM Matthieu Baerts
> > > > <matthieu.baerts@tessares.net> wrote:
> > > > >
> > > > > Hi Lorenzo,
> > > > >
> > > > > On 09/03/2023 13:25, Lorenzo Bianconi wrote:
> > > > > > Take into account tx/rx queues reconfiguration setting device
> > > > > > xdp_features flag. Moreover consider NETIF_F_GRO flag in order to enable
> > > > > > ndo_xdp_xmit callback.
> > > > > >
> > > > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > >
> > > > > Thank you for the modification.
> > > > >
> > > > > Unfortunately, 'git bisect' just told me this modification is the origin
> > > > > of a new WARN when using veth in a netns:
> > > > >
> > > > >
> > > > > ###################### 8< ######################
> > > > >
> > > > > =============================
> > > > > WARNING: suspicious RCU usage
> > > > > 6.3.0-rc1-00144-g064d70527aaa #149 Not tainted
> > > > > -----------------------------
> > > > > drivers/net/veth.c:1265 suspicious rcu_dereference_check() usage!
> > > > >
> > > > > other info that might help us debug this:
> > > > >
> > > >
> > > > Same observation here, I am releasing a syzbot report with a repro.
> > > >
> > > >
> > >
> > > I guess a fix would be:
> > >
> >
> > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Can you submit a formal fix ?

ack, will do.

Regards,
Lorenzo

> 
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v2 6/8] veth: take into account device reconfiguration for xdp_features flag

[Martin KaFai Lau]

On 3/9/23 4:25 AM, Lorenzo Bianconi wrote:
> +static void veth_set_xdp_features(struct net_device *dev)
> +{
> +	struct veth_priv *priv = netdev_priv(dev);
> +	struct net_device *peer;
> +
> +	peer = rcu_dereference(priv->peer);
> +	if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
> +		xdp_features_t val = NETDEV_XDP_ACT_BASIC |
> +				     NETDEV_XDP_ACT_REDIRECT |
> +				     NETDEV_XDP_ACT_RX_SG;
> +
> +		if (priv->_xdp_prog || veth_gro_requested(dev))
> +			val |= NETDEV_XDP_ACT_NDO_XMIT |
> +			       NETDEV_XDP_ACT_NDO_XMIT_SG;

This broke the xdp_do_redirect selftest. The bpf CI is consistently failing at:

test_xdp_do_redirect:FAIL:veth_src query_opts.feature_flags unexpected veth_src 
query_opts.feature_flags: actual 35 != expected 103

Please address it asap.

[PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Lorenzo Bianconi]

Take into account LRO and GRO configuration setting device xdp_features
flag. Consider channel rq_wq_type enabling rx scatter-gatter support in
xdp_features flag and disable NETDEV_XDP_ACT_NDO_XMIT_SG since it is not
supported yet by the driver.
Moreover always enable NETDEV_XDP_ACT_NDO_XMIT as the ndo_xdp_xmit
callback does not require to load a dummy xdp program on the NIC.

Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
Co-developed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  1 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 10 ++++-
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 37 +++++++++++++------
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  3 ++
 4 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 88460b7796e5..4276c6eb6820 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -1243,6 +1243,7 @@ void mlx5e_build_nic_params(struct mlx5e_priv *priv, struct mlx5e_xsk *xsk, u16
 void mlx5e_rx_dim_work(struct work_struct *work);
 void mlx5e_tx_dim_work(struct work_struct *work);
 
+void mlx5e_set_xdp_feature(struct net_device *netdev);
 netdev_features_t mlx5e_features_check(struct sk_buff *skb,
 				       struct net_device *netdev,
 				       netdev_features_t features);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 7708acc9b2ab..79fd21ecb9cb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1985,6 +1985,7 @@ static int set_pflag_rx_striding_rq(struct net_device *netdev, bool enable)
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 	struct mlx5_core_dev *mdev = priv->mdev;
 	struct mlx5e_params new_params;
+	int err;
 
 	if (enable) {
 		/* Checking the regular RQ here; mlx5e_validate_xsk_param called
@@ -2005,7 +2006,14 @@ static int set_pflag_rx_striding_rq(struct net_device *netdev, bool enable)
 	MLX5E_SET_PFLAG(&new_params, MLX5E_PFLAG_RX_STRIDING_RQ, enable);
 	mlx5e_set_rq_type(mdev, &new_params);
 
-	return mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	err = mlx5e_safe_switch_params(priv, &new_params, NULL, NULL, true);
+	if (err)
+		return err;
+
+	/* update XDP supported features */
+	mlx5e_set_xdp_feature(netdev);
+
+	return 0;
 }
 
 static int set_pflag_rx_no_csum_complete(struct net_device *netdev, bool enable)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 76a9c5194a70..51b5f3cca504 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4004,6 +4004,25 @@ static int mlx5e_handle_feature(struct net_device *netdev,
 	return 0;
 }
 
+void mlx5e_set_xdp_feature(struct net_device *netdev)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+	struct mlx5e_params *params = &priv->channels.params;
+	xdp_features_t val;
+
+	if (params->packet_merge.type != MLX5E_PACKET_MERGE_NONE) {
+		xdp_clear_features_flag(netdev);
+		return;
+	}
+
+	val = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
+	      NETDEV_XDP_ACT_XSK_ZEROCOPY |
+	      NETDEV_XDP_ACT_NDO_XMIT;
+	if (params->rq_wq_type == MLX5_WQ_TYPE_CYCLIC)
+		val |= NETDEV_XDP_ACT_RX_SG;
+	xdp_set_features_flag(netdev, val);
+}
+
 int mlx5e_set_features(struct net_device *netdev, netdev_features_t features)
 {
 	netdev_features_t oper_features = features;
@@ -4030,6 +4049,9 @@ int mlx5e_set_features(struct net_device *netdev, netdev_features_t features)
 		return -EINVAL;
 	}
 
+	/* update XDP supported features */
+	mlx5e_set_xdp_feature(netdev);
+
 	return 0;
 }
 
@@ -4761,13 +4783,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 	if (old_prog)
 		bpf_prog_put(old_prog);
 
-	if (reset) {
-		if (prog)
-			xdp_features_set_redirect_target(netdev, true);
-		else
-			xdp_features_clear_redirect_target(netdev);
-	}
-
 	if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset)
 		goto unlock;
 
@@ -5163,13 +5178,10 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 	netdev->features         |= NETIF_F_HIGHDMA;
 	netdev->features         |= NETIF_F_HW_VLAN_STAG_FILTER;
 
-	netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
-			       NETDEV_XDP_ACT_XSK_ZEROCOPY |
-			       NETDEV_XDP_ACT_RX_SG;
-
 	netdev->priv_flags       |= IFF_UNICAST_FLT;
 
 	netif_set_tso_max_size(netdev, GSO_MAX_SIZE);
+	mlx5e_set_xdp_feature(netdev);
 	mlx5e_set_netdev_dev_addr(netdev);
 	mlx5e_macsec_build_netdev(priv);
 	mlx5e_ipsec_build_netdev(priv);
@@ -5241,6 +5253,9 @@ static int mlx5e_nic_init(struct mlx5_core_dev *mdev,
 		mlx5_core_err(mdev, "TLS initialization failed, %d\n", err);
 
 	mlx5e_health_create_reporters(priv);
+	/* update XDP supported features */
+	mlx5e_set_xdp_feature(netdev);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 9b9203443085..43fd12fb87b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -747,6 +747,9 @@ static void mlx5e_build_rep_params(struct net_device *netdev)
 	/* RQ */
 	mlx5e_build_rq_params(mdev, params);
 
+	/* update XDP supported features */
+	mlx5e_set_xdp_feature(netdev);
+
 	/* CQ moderation params */
 	params->rx_dim_enabled = MLX5_CAP_GEN(mdev, cq_moderation);
 	mlx5e_set_rx_cq_mode_params(params, cq_period_mode);
-- 
2.39.2

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Jakub Kicinski]

On Thu,  9 Mar 2023 13:25:31 +0100 Lorenzo Bianconi wrote:
> Take into account LRO and GRO configuration setting device xdp_features
> flag. Consider channel rq_wq_type enabling rx scatter-gatter support in
> xdp_features flag and disable NETDEV_XDP_ACT_NDO_XMIT_SG since it is not
> supported yet by the driver.
> Moreover always enable NETDEV_XDP_ACT_NDO_XMIT as the ndo_xdp_xmit
> callback does not require to load a dummy xdp program on the NIC.
> 
> Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
> Co-developed-by: Tariq Toukan <tariqt@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

This one hits ASSERT_RTNL(), I think. Don't we need something like:

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 87e654b7d06c..5722a1fc6e9e 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
                return;
 
        dev->xdp_features = val;
+
+       if (dev->reg_state < NETREG_REGISTERED)
+               return;
        call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
 }
 EXPORT_SYMBOL_GPL(xdp_set_features_flag);

? The notifiers are not needed until the device is actually live.

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Jakub Kicinski]

On Wed, 15 Mar 2023 16:39:00 -0700 Jakub Kicinski wrote:
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 87e654b7d06c..5722a1fc6e9e 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
>                 return;
>  
>         dev->xdp_features = val;
> +
> +       if (dev->reg_state < NETREG_REGISTERED)
> +               return;
>         call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
>  }
>  EXPORT_SYMBOL_GPL(xdp_set_features_flag);
> 
> ? The notifiers are not needed until the device is actually live.

I think so.. let me send a full patch.

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Saeed Mahameed]

On 15 Mar 17:29, Jakub Kicinski wrote:
>On Wed, 15 Mar 2023 16:39:00 -0700 Jakub Kicinski wrote:
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 87e654b7d06c..5722a1fc6e9e 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -781,6 +781,9 @@ void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
>>                 return;
>>
>>         dev->xdp_features = val;
>> +
>> +       if (dev->reg_state < NETREG_REGISTERED)
>> +               return;
>>         call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
>>  }
>>  EXPORT_SYMBOL_GPL(xdp_set_features_flag);
>>
>> ? The notifiers are not needed until the device is actually live.
>
>I think so.. let me send a full patch.

We have an  internal version of a fix, Tariq is finalizing some review
comments and we will be posting it ASAP.

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Jakub Kicinski]

On Wed, 15 Mar 2023 19:46:10 -0700 Saeed Mahameed wrote:
> >I think so.. let me send a full patch.  
> 
> We have an  internal version of a fix, Tariq is finalizing some review
> comments and we will be posting it ASAP.

Ah, I already posted. Does it look different?

https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/

I wanted to make sure that it's ready for tomorrow's PR

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Saeed Mahameed]

On 15 Mar 19:53, Jakub Kicinski wrote:
>On Wed, 15 Mar 2023 19:46:10 -0700 Saeed Mahameed wrote:
>> >I think so.. let me send a full patch.
>>
>> We have an  internal version of a fix, Tariq is finalizing some review
>> comments and we will be posting it ASAP.
>
>Ah, I already posted. Does it look different?
>

Yes, completely different, Tariq's fix is in mlx5 only.
He splits the xdp  feature setting into two functions, 
one to initialize the netdev's xdp before registration,
and another one to update xpd features and call the notifier in the
"after" registration set_features flows.

I like our solution more, since it's more explicit and doesn't require
patching xdp stack because mlx5 abused xdp_set_features_flag, unless other
drivers have the same issue.

>https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/
>

I don't see anything wrong with your patch though.. it also looks more
elegant, i dunno, I will let you decide, here's our patch:
https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=testing/xdp-features-fix&id=72e2266525948ba1498e6a3f2d63ea10d5ee86f5


>I wanted to make sure that it's ready for tomorrow's PR

Re: [PATCH net v2 7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag

[Jakub Kicinski]

On Wed, 15 Mar 2023 22:03:27 -0700 Saeed Mahameed wrote:
> Yes, completely different, Tariq's fix is in mlx5 only.
> He splits the xdp  feature setting into two functions, 
> one to initialize the netdev's xdp before registration,
> and another one to update xpd features and call the notifier in the
> "after" registration set_features flows.
> 
> I like our solution more, since it's more explicit and doesn't require
> patching xdp stack because mlx5 abused xdp_set_features_flag, unless other
> drivers have the same issue.

I reckon there's nothing wrong with calling xdp_set_features_flag()
before registration, I didn't do much research, but
netif_set_real_num_*x_queues() come to mind, and they can be called
at any time. The stack should act accordingly.

Many drivers may need to work around an issue which can be handled
centrally.

Let's see if Lorenzo disagrees.

> >https://patchwork.kernel.org/project/netdevbpf/patch/20230316002903.492497-1-kuba@kernel.org/
> >  
> 
> I don't see anything wrong with your patch though.. it also looks more
> elegant, i dunno, I will let you decide, here's our patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=testing/xdp-features-fix&id=72e2266525948ba1498e6a3f2d63ea10d5ee86f5

[PATCH net v2 8/8] mvpp2: take care of xdp_features when reconfiguring queues

[Lorenzo Bianconi]

From: Matteo Croce <teknoraver@meta.com>

XDP is supported only if enough queues are present, so when reconfiguring
the queues set xdp_features accordingly.

Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features")
Suggested-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Matteo Croce <teknoraver@meta.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 9b4ecbe4f36d..3ea00bc9b91c 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4996,6 +4996,14 @@ static int mvpp2_bm_switch_buffers(struct mvpp2 *priv, bool percpu)
 
 	for (i = 0; i < priv->port_count; i++) {
 		port = priv->port_list[i];
+		if (percpu && port->ntxqs >= num_possible_cpus() * 2)
+			xdp_set_features_flag(port->dev,
+					      NETDEV_XDP_ACT_BASIC |
+					      NETDEV_XDP_ACT_REDIRECT |
+					      NETDEV_XDP_ACT_NDO_XMIT);
+		else
+			xdp_clear_features_flag(port->dev);
+
 		mvpp2_swf_bm_pool_init(port);
 		if (status[i])
 			mvpp2_open(port->dev);
@@ -6863,13 +6871,14 @@ static int mvpp2_port_probe(struct platform_device *pdev,
 
 	if (!port->priv->percpu_pools)
 		mvpp2_set_hw_csum(port, port->pool_long->id);
+	else if (port->ntxqs >= num_possible_cpus() * 2)
+		dev->xdp_features = NETDEV_XDP_ACT_BASIC |
+				    NETDEV_XDP_ACT_REDIRECT |
+				    NETDEV_XDP_ACT_NDO_XMIT;
 
 	dev->vlan_features |= features;
 	netif_set_tso_max_segs(dev, MVPP2_MAX_TSO_SEGS);
 
-	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
-			    NETDEV_XDP_ACT_NDO_XMIT;
-
 	dev->priv_flags |= IFF_UNICAST_FLT;
 
 	/* MTU range: 68 - 9704 */
-- 
2.39.2

Re: [PATCH net v2 0/8] update xdp_features flag according to NIC re-configuration

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  9 Mar 2023 13:25:24 +0100 you wrote:
> Changes since v1:
> - rebase on top of net tree
> - remove NETDEV_XDP_ACT_NDO_XMIT_SG support in mlx5e driver
> - always enable NETDEV_XDP_ACT_NDO_XMIT support in mlx5e driver
> 
> Lorenzo Bianconi (7):
>   tools: ynl: fix render-max for flags definition
>   tools: ynl: fix get_mask utility routine
>   xdp: add xdp_set_features_flag utility routine
>   net: thunderx: take into account xdp_features setting tx/rx queues
>   net: ena: take into account xdp_features setting tx/rx queues
>   veth: take into account device reconfiguration for xdp_features flag
>   net/mlx5e: take into account device reconfiguration for xdp_features
>     flag
> 
> [...]

Here is the summary with links:
  - [net,v2,1/8] tools: ynl: fix render-max for flags definition
    https://git.kernel.org/netdev/net/c/8f76a4f80fba
  - [net,v2,2/8] tools: ynl: fix get_mask utility routine
    https://git.kernel.org/netdev/net/c/bf51d27704c9
  - [net,v2,3/8] xdp: add xdp_set_features_flag utility routine
    https://git.kernel.org/netdev/net/c/f85949f98206
  - [net,v2,4/8] net: thunderx: take into account xdp_features setting tx/rx queues
    https://git.kernel.org/netdev/net/c/3c249fe4de16
  - [net,v2,5/8] net: ena: take into account xdp_features setting tx/rx queues
    https://git.kernel.org/netdev/net/c/7aa6dc351b92
  - [net,v2,6/8] veth: take into account device reconfiguration for xdp_features flag
    https://git.kernel.org/netdev/net/c/fccca038f300
  - [net,v2,7/8] net/mlx5e: take into account device reconfiguration for xdp_features flag
    https://git.kernel.org/netdev/net/c/4d5ab0ad964d
  - [net,v2,8/8] mvpp2: take care of xdp_features when reconfiguring queues
    https://git.kernel.org/netdev/net/c/481e96fc1307

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html