Skip to contentSkip to conversation

ixgbe: prevent driver configuration changes while XDP is loaded

PATCH New John Fastabend 4 participants 7 messages May 2, 2017 – Jun 6, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +17 -0
index ee20a2b..156357e 100644
@@ -6437,6 +6437,19 @@ static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter)
 static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	int i, frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+
+	/* If XDP is running enforce MTU limitations */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		struct ixgbe_ring *ring = adapter->rx_ring[i];
+
+		if (frame_size > ixgbe_rx_bufsz(ring)) {
+			e_warn(probe,
+			       "Setting MTU > %i with XDP is not supported\n",
+			       ixgbe_rx_bufsz(ring));
+			return -EINVAL;
+		}
+	}
 
 	/*
 	 * For 82599EB we cannot allow legacy VFs to enable their receive
@@ -9291,6 +9304,10 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
 	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
 		features &= ~NETIF_F_LRO;
 
+	/* If XDP is enabled we can not enable LRO */
+	if (adapter->xdp_prog)
+		features &= ~NETIF_F_LRO;
+
 	return features;
 }
 
#1 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of John Fastabend
Sent: Monday, May 1, 2017 10:07 PM
To: [email protected]
Subject: [Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

---

With this patch applied, we are seeing issue where setting initial MTU >= 1515 fails with Error: Setting MTU > 1536 with XDP is not supported, but setting MTU to 1514 , we can now set MTU till 3050. If we set anything greater than 3050 it errors out with the following message: Setting MTU > 3072 with XDP is not supported.

Is there a way to see if XDP is enabled and is XDP is supposed to be enabled by default on ixgbe ? only info about XDP seen on platform is in dmesg as follows.

[    3.267909] ixgbe 0000:04:00.0: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    3.724686] ixgbe 0000:04:00.1: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    4.186271] ixgbe 0000:06:00.0: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0
[    4.644388] ixgbe 0000:06:00.1: Multiqueue Enabled: Rx Queue count = 12, Tx Queue count = 12 XDP Queue count = 0

When using latest iproute2 tool to disable xdp ( ip link set ethX xdp off) , there is no error reported by iproute2 and nothing is reported in dmesg but we were still unable to set MTU > = 3051.

#2 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

In reply to John Fastabend
Quoted 23 lines · John Fastabend

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4 -0
index ee20a2b..156357e 100644
@@ -6437,6 +6437,19 @@ static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter)
 static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
 {
        struct ixgbe_adapter *adapter = netdev_priv(netdev);
+       int i, frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+
+       /* If XDP is running enforce MTU limitations */
+       for (i = 0; i < adapter->num_rx_queues; i++) {

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter) · line 6453

Quoted 10 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +9 -0
+               struct ixgbe_ring *ring = adapter->rx_ring[i];
+
+               if (frame_size > ixgbe_rx_bufsz(ring)) {
+                       e_warn(probe,
+                              "Setting MTU > %i with XDP is not supported\n",
+                              ixgbe_rx_bufsz(ring));
+                       return -EINVAL;
+               }
+       }
 

There are a few issues with this piece.

First the Rx buffer size gets updated depending on if we are using jumbo frames or not. We either use 1.5K or 3K to hold the contents of the frame. Instead of using ixgbe_rx_bufsz you might just make it so that you use IXGBE_MAX_FRAME_BUILD_SKB if the adapter flag for LEGACY_RX is not set, otherwise you can probably just use IXGBE_RXBUFFER_2K.

Also you should only be performing this check if xdp_prog is set and it doesn't look like you are checking for that.

Quoted 18 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4 -0
        /*
         * For 82599EB we cannot allow legacy VFs to enable their receive
@@ -9291,6 +9304,10 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
        if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
                features &= ~NETIF_F_LRO;
 
+       /* If XDP is enabled we can not enable LRO */
+       if (adapter->xdp_prog)
+               features &= ~NETIF_F_LRO;
+
        return features;
 }
 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

#3 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

In reply to John Fastabend
Quoted 12 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of John Fastabend
Sent: Monday, May 1, 2017 10:07 PM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [PATCH] ixgbe: prevent driver configuration changes while XDP is loaded

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

Quoted 6 lines

This patch adds checks to ensure config changes are valid.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
#4 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

In reply to John Fastabend
Quoted 11 lines · John Fastabend

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
+++++++++++++++++
 1 file changed, 17 insertions(+)

I know you have moved onto a new job, which is probably taking up most
of your time. I just need to know if you will be re-spinning this
patch based on the feedback from Alex.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170606/8dd16407/attachment.asc>

#5 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

Quoted 13 lines · Jeff Kirsher
Quoted 11 lines · John Fastabend

XDP checks to ensure the MTU is valid and LRO is disabled when it is loaded. But user configuration after XDP is loaded could change these and cause a misconfiguration.

This patch adds checks to ensure config changes are valid.

---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
+++++++++++++++++
 1 file changed, 17 insertions(+)
Quoted 4 lines

I know you have moved onto a new job, which is probably taking up most of your time. I just need to know if you will be re-spinning this patch based on the feedback from Alex.

I planned to get the xdp_redirect patches updated and sent out in the next few weeks. When I get around to that I'll also push an update to this assuming no one beats me to it.

Thanks,
John

#6 lore raw

PATCH 1/1 ixgbe: prevent driver configuration changes while XDP is loaded

Quoted 22 lines · John Fastabend
Quoted 15 lines · Jeff Kirsher
Quoted 6 lines · John Fastabend

XDP checks to ensure the MTU is valid and LRO is disabled when it is
loaded. But user configuration after XDP is loaded could change these
and cause a misconfiguration.

Quoted 7 lines

This patch adds checks to ensure config changes are valid.

---
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   17
+++++++++++++++++
  1 file changed, 17 insertions(+)
Quoted 5 lines

I know you have moved onto a new job, which is probably taking up most
of your time. I just need to know if you will be re-spinning this patch based on the feedback from Alex.

Quoted 3 lines

I planned to get the xdp_redirect patches updated and sent out in the next few weeks. When I get around to that I'll also push an update to this assuming no one beats me to it.

Ok thanks for the update, I will drop the current patch and will await
an updated version.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170606/1d7b3c21/attachment.asc>