| Message ID | 20240806221533.3360049-1-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] igb: cope with large MAX_SKB_FRAGS. | expand |
On Tue, Aug 06, 2024 at 03:15:31PM -0700, Tony Nguyen wrote: > From: Paolo Abeni <pabeni@redhat.com> > > Sabrina reports that the igb driver does not cope well with large > MAX_SKB_FRAG values: setting MAX_SKB_FRAG to 45 causes payload > corruption on TX. > > An easy reproducer is to run ssh to connect to the machine. With > MAX_SKB_FRAGS=17 it works, with MAX_SKB_FRAGS=45 it fails. any splat? > > The root cause of the issue is that the driver does not take into > account properly the (possibly large) shared info size when selecting > the ring layout, and will try to fit two packets inside the same 4K > page even when the 1st fraglist will trump over the 2nd head. > > Address the issue forcing the driver to fit a single packet per page, > leaving there enough room to store the (currently) largest possible > skb_shared_info. > > Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS") > Reported-by: Jan Tluka <jtluka@redhat.com> > Reported-by: Jirka Hladky <jhladky@redhat.com> > Reported-by: Sabrina Dubroca <sd@queasysnail.net> Where was this reported? > Tested-by: Sabrina Dubroca <sd@queasysnail.net> > Tested-by: Corinna Vinschen <vinschen@redhat.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > Reviewed-by: Eric Dumazet <edumazet@google.com> > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > --- > iwl-net: https://lore.kernel.org/intel-wired-lan/20240718085633.1285322-1-vinschen@redhat.com/ > > drivers/net/ethernet/intel/igb/igb_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index 11be39f435f3..232d6cb836a9 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -4808,6 +4808,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter, > > #if (PAGE_SIZE < 8192) > if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB || > + SKB_HEAD_ALIGN(adapter->max_frame_size) > (PAGE_SIZE / 2) || We should address IGB_2K_TOO_SMALL_WITH_PADDING for this case. I'll think about it tomorrow. > rd32(E1000_RCTL) & E1000_RCTL_SBP) > set_ring_uses_large_buffer(rx_ring); > #endif > -- > 2.42.0 > >
On Wed, Aug 07, 2024 at 06:09:20PM +0200, Maciej Fijalkowski wrote: > On Tue, Aug 06, 2024 at 03:15:31PM -0700, Tony Nguyen wrote: > > From: Paolo Abeni <pabeni@redhat.com> > > > > Sabrina reports that the igb driver does not cope well with large > > MAX_SKB_FRAG values: setting MAX_SKB_FRAG to 45 causes payload > > corruption on TX. > > > > An easy reproducer is to run ssh to connect to the machine. With > > MAX_SKB_FRAGS=17 it works, with MAX_SKB_FRAGS=45 it fails. > > any splat? > > > > > The root cause of the issue is that the driver does not take into > > account properly the (possibly large) shared info size when selecting > > the ring layout, and will try to fit two packets inside the same 4K > > page even when the 1st fraglist will trump over the 2nd head. > > > > Address the issue forcing the driver to fit a single packet per page, > > leaving there enough room to store the (currently) largest possible > > skb_shared_info. > > > > Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS") > > Reported-by: Jan Tluka <jtluka@redhat.com> > > Reported-by: Jirka Hladky <jhladky@redhat.com> > > Reported-by: Sabrina Dubroca <sd@queasysnail.net> > > Where was this reported? > > > Tested-by: Sabrina Dubroca <sd@queasysnail.net> > > Tested-by: Corinna Vinschen <vinschen@redhat.com> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > Reviewed-by: Eric Dumazet <edumazet@google.com> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel) > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > --- > > iwl-net: https://lore.kernel.org/intel-wired-lan/20240718085633.1285322-1-vinschen@redhat.com/ > > > > drivers/net/ethernet/intel/igb/igb_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > index 11be39f435f3..232d6cb836a9 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > @@ -4808,6 +4808,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter, > > > > #if (PAGE_SIZE < 8192) > > if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB || > > + SKB_HEAD_ALIGN(adapter->max_frame_size) > (PAGE_SIZE / 2) || > > We should address IGB_2K_TOO_SMALL_WITH_PADDING for this case. I'll think > about it tomorrow. Actually from what I currently understand IGB_2K_TOO_SMALL_WITH_PADDING will give us 'true' for case you are addressing so we could reuse it here? > > > rd32(E1000_RCTL) & E1000_RCTL_SBP) > > set_ring_uses_large_buffer(rx_ring); > > #endif > > -- > > 2.42.0 > > > >
On Aug 8 20:20, Maciej Fijalkowski wrote: > On Wed, Aug 07, 2024 at 06:09:20PM +0200, Maciej Fijalkowski wrote: > > On Tue, Aug 06, 2024 at 03:15:31PM -0700, Tony Nguyen wrote: > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > > index 11be39f435f3..232d6cb836a9 100644 > > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > > @@ -4808,6 +4808,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter, > > > > > > #if (PAGE_SIZE < 8192) > > > if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB || > > > + SKB_HEAD_ALIGN(adapter->max_frame_size) > (PAGE_SIZE / 2) || > > > > We should address IGB_2K_TOO_SMALL_WITH_PADDING for this case. I'll think > > about it tomorrow. > > Actually from what I currently understand IGB_2K_TOO_SMALL_WITH_PADDING > will give us 'true' for case you are addressing so we could reuse it here? Well, IGB_2K_TOO_SMALL_WITH_PADDING is a constant expression evaluated at build time. The SKB_HEAD_ALIGN expression is dynamically computed, basically depending on the MTU. IGB_2K_TOO_SMALL_WITH_PADDING switches from false to true with MAX_SKB_FRAGS set to 22 or more. So with MAX_SKB_FRAGS set to 17, IGB_2K_TOO_SMALL_WITH_PADDING is false, while the SKB_HEAD_ALIGN expression is true for MTUs >= 1703. With MAX_SKB_FRAGS set to 45, IGB_2K_TOO_SMALL_WITH_PADDING is true, the SKB_HEAD_ALIGN expression is true for MTUs >= 1255. Given that, IGB_2K_TOO_SMALL_WITH_PADDING might be the more careful check. Do you want me to send a v2? Thanks, Corinna
On Fri, Aug 09, 2024 at 02:16:20PM +0200, Corinna Vinschen wrote: > On Aug 8 20:20, Maciej Fijalkowski wrote: > > On Wed, Aug 07, 2024 at 06:09:20PM +0200, Maciej Fijalkowski wrote: > > > On Tue, Aug 06, 2024 at 03:15:31PM -0700, Tony Nguyen wrote: > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > > > index 11be39f435f3..232d6cb836a9 100644 > > > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > > > @@ -4808,6 +4808,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter, > > > > > > > > #if (PAGE_SIZE < 8192) > > > > if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB || > > > > + SKB_HEAD_ALIGN(adapter->max_frame_size) > (PAGE_SIZE / 2) || > > > > > > We should address IGB_2K_TOO_SMALL_WITH_PADDING for this case. I'll think > > > about it tomorrow. > > > > Actually from what I currently understand IGB_2K_TOO_SMALL_WITH_PADDING > > will give us 'true' for case you are addressing so we could reuse it here? > > Well, IGB_2K_TOO_SMALL_WITH_PADDING is a constant expression evaluated > at build time. The SKB_HEAD_ALIGN expression is dynamically computed, > basically depending on the MTU. > > IGB_2K_TOO_SMALL_WITH_PADDING switches from false to true with > MAX_SKB_FRAGS set to 22 or more. > > So with MAX_SKB_FRAGS set to 17, IGB_2K_TOO_SMALL_WITH_PADDING is false, > while the SKB_HEAD_ALIGN expression is true for MTUs >= 1703. MTU >= 1703 will trigger the current adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB statement. > > With MAX_SKB_FRAGS set to 45, IGB_2K_TOO_SMALL_WITH_PADDING is true, > the SKB_HEAD_ALIGN expression is true for MTUs >= 1255. > > Given that, IGB_2K_TOO_SMALL_WITH_PADDING might be the more careful > check. Do you want me to send a v2? Using IGB_2K_TOO_SMALL_WITH_PADDING here aligns it with igb_skb_pad() when computing headroom, so if you don't mind maybe it will be cleaner to reuse it? > > > Thanks, > Corinna >
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 11be39f435f3..232d6cb836a9 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -4808,6 +4808,7 @@ static void igb_set_rx_buffer_len(struct igb_adapter *adapter, #if (PAGE_SIZE < 8192) if (adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB || + SKB_HEAD_ALIGN(adapter->max_frame_size) > (PAGE_SIZE / 2) || rd32(E1000_RCTL) & E1000_RCTL_SBP) set_ring_uses_large_buffer(rx_ring); #endif