| Message ID | 20240227192704.376176-1-e.velu@criteo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | i40e: Prevent setting MTU if greater than MFS | expand |
Tue, Feb 27, 2024 at 08:27:03PM CET, erwanaliasr1@gmail.com wrote: >Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is >setting the MFS to 0x600 = 1536. > >At boot time the i40e driver complains about it with >the following message but continues. > > MFS for port 1 has been set below the default: 600 > >If the MTU size is increased, the driver accept it but large packets will not >be processed by the firmware generating tx_errors. The issue is pretty >silent for users. i.e doing TCP in such context will generates lots of >retransmissions until the proper window size (below 1500) will be used. > >To fix this case, it would have been ideal to increase the MFS, >via i40e_aqc_opc_set_mac_config, but I didn't found a reliable way to do it. > >At least, this commit prevents setting up an MTU greater than the current MFS. >It will avoid being in the position of having an MTU set to 9000 on the >netdev with a firmware refusing packets larger than 1536. > >A typical trace looks like the following : >[ 377.548696] i40e 0000:5d:00.0 eno5: Error changing mtu to 9000 which is greater than the current mfs: 1536 > >Signed-off-by: Erwan Velu <e.velu@criteo.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Next time, could you please indicate the target tree in the patch subject prefix, like this: "[patch net-next] xxx" ? >--- > drivers/net/ethernet/intel/i40e/i40e_main.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c >index 54eb55464e31..14fc70d854d3 100644 >--- a/drivers/net/ethernet/intel/i40e/i40e_main.c >+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c >@@ -2950,7 +2950,7 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu) > struct i40e_netdev_priv *np = netdev_priv(netdev); > struct i40e_vsi *vsi = np->vsi; > struct i40e_pf *pf = vsi->back; >- int frame_size; >+ int frame_size, mfs; > > frame_size = i40e_max_vsi_frame_size(vsi, vsi->xdp_prog); > if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) { >@@ -2959,6 +2959,13 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu) > return -EINVAL; > } > >+ mfs = pf->hw.phy.link_info.max_frame_size; >+ if (new_mtu > mfs) { >+ netdev_err(netdev, "Error changing mtu to %d which is greater than the current mfs: %d\n", >+ new_mtu, mfs); >+ return -EINVAL; >+ } >+ > netdev_dbg(netdev, "changing MTU from %d to %d\n", > netdev->mtu, new_mtu); > netdev->mtu = new_mtu; >-- >2.43.2 > >
On 2/27/2024 11:27 AM, Erwan Velu wrote: > Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is > setting the MFS to 0x600 = 1536. > > At boot time the i40e driver complains about it with > the following message but continues. > > MFS for port 1 has been set below the default: 600 > > If the MTU size is increased, the driver accept it but large packets will not > be processed by the firmware generating tx_errors. The issue is pretty > silent for users. i.e doing TCP in such context will generates lots of > retransmissions until the proper window size (below 1500) will be used. > > To fix this case, it would have been ideal to increase the MFS, > via i40e_aqc_opc_set_mac_config, but I didn't found a reliable way to do it. > > At least, this commit prevents setting up an MTU greater than the current MFS. > It will avoid being in the position of having an MTU set to 9000 on the > netdev with a firmware refusing packets larger than 1536. > > A typical trace looks like the following : > [ 377.548696] i40e 0000:5d:00.0 eno5: Error changing mtu to 9000 which is greater than the current mfs: 1536 > > Signed-off-by: Erwan Velu <e.velu@criteo.com> The Author and Sign-off needs to be fixed; they don't match. WARNING: From:/Signed-off-by: email address mismatch: 'From: Erwan Velu <erwanaliasr1@gmail.com>' != 'Signed-off-by: Erwan Velu <e.velu@criteo.com>' Thanks, Tony
Le lun. 4 mars 2024 à 23:10, Tony Nguyen <anthony.l.nguyen@intel.com> a écrit : > > Signed-off-by: Erwan Velu <e.velu@criteo.com> > > The Author and Sign-off needs to be fixed; they don't match. > > WARNING: From:/Signed-off-by: email address mismatch: 'From: Erwan Velu > <erwanaliasr1@gmail.com>' != 'Signed-off-by: Erwan Velu <e.velu@criteo.com>' Yeah, I have a complicated email setup between my personal and professional emails. I'll see how I can fix that. I was also wondering if I shouldn't subtract I40E_PACKET_HDR_PAD from the mfs to be more accurate, can you confirm this ? If one can have a look at what is the exact procedure to fix the MFS size when too small, that would be lovely/ideal in addition to my patch.
On 3/4/2024 6:22 PM, Erwan Velu wrote: > I was also wondering if I shouldn't subtract I40E_PACKET_HDR_PAD from > the mfs to be more accurate, can you confirm this ? Yes, you're correct, we should take the packet header out however... > If one can have a look at what is the exact procedure to fix the MFS > size when too small, that would be lovely/ideal in addition to my > patch. ... as you allude to here, we should resolve the MFS size issue. You were on that right track (as mentioned in your commit message) > To fix this case, it would have been ideal to increase the MFS, > via i40e_aqc_opc_set_mac_config, but I didn't found a reliable way to do it. The ice driver is doing this with the ice_aq_set_mac_cfg() call. You should be able to model the same here. Thanks, Tony
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 54eb55464e31..14fc70d854d3 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -2950,7 +2950,7 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu) struct i40e_netdev_priv *np = netdev_priv(netdev); struct i40e_vsi *vsi = np->vsi; struct i40e_pf *pf = vsi->back; - int frame_size; + int frame_size, mfs; frame_size = i40e_max_vsi_frame_size(vsi, vsi->xdp_prog); if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) { @@ -2959,6 +2959,13 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu) return -EINVAL; } + mfs = pf->hw.phy.link_info.max_frame_size; + if (new_mtu > mfs) { + netdev_err(netdev, "Error changing mtu to %d which is greater than the current mfs: %d\n", + new_mtu, mfs); + return -EINVAL; + } + netdev_dbg(netdev, "changing MTU from %d to %d\n", netdev->mtu, new_mtu); netdev->mtu = new_mtu;
Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is setting the MFS to 0x600 = 1536. At boot time the i40e driver complains about it with the following message but continues. MFS for port 1 has been set below the default: 600 If the MTU size is increased, the driver accept it but large packets will not be processed by the firmware generating tx_errors. The issue is pretty silent for users. i.e doing TCP in such context will generates lots of retransmissions until the proper window size (below 1500) will be used. To fix this case, it would have been ideal to increase the MFS, via i40e_aqc_opc_set_mac_config, but I didn't found a reliable way to do it. At least, this commit prevents setting up an MTU greater than the current MFS. It will avoid being in the position of having an MTU set to 9000 on the netdev with a firmware refusing packets larger than 1536. A typical trace looks like the following : [ 377.548696] i40e 0000:5d:00.0 eno5: Error changing mtu to 9000 which is greater than the current mfs: 1536 Signed-off-by: Erwan Velu <e.velu@criteo.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)