| Message ID | 20170502050634.7459.50968.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Kirsher |
| Headers | show |
-----Original Message----- From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of John Fastabend Sent: Monday, May 1, 2017 10:07 PM To: intel-wired-lan@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. This patch adds checks to ensure config changes are valid. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- 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.
On Mon, May 1, 2017 at 10:06 PM, John Fastabend <john.fastabend@gmail.com> wrote: > 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. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index ee20a2b..156357e 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -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; > + } > + } > 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. > /* > * 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@lists.osuosl.org > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of John Fastabend > Sent: Monday, May 1, 2017 10:07 PM > To: intel-wired-lan@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. > > This patch adds checks to ensure config changes are valid. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote: > 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. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > 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.
On 06/06/2017 01:14 PM, Jeff Kirsher wrote: > On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote: >> 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. >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> 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. > 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
On Tue, 2017-06-06 at 13:17 -0700, John Fastabend wrote: > On 06/06/2017 01:14 PM, Jeff Kirsher wrote: > > On Mon, 2017-05-01 at 22:06 -0700, John Fastabend wrote: > > > 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. > > > > > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > > > --- > > > 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. > > > > 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.
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index ee20a2b..156357e 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -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; }
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. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)