| Message ID | 20240423102446.901450-1-vinschen@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igb: cope with large MAX_SKB_FRAGS. | expand |
Tue, Apr 23, 2024 at 12:24:46PM CEST, vinschen@redhat.com 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. > >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_FRAG") >Reported-by: Jan Tluka <jtluka@redhat.com> >Reported-by: Jirka Hladky <jhladky@redhat.com> >Reported-by: Sabrina Dubroca <sd@queasysnail.net> >Tested-by: Sabrina Dubroca <sd@queasysnail.net> >Tested-by: Corinna Vinschen <vinschen@redhat.com> >Signed-off-by: Paolo Abeni <pabeni@redhat.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Next time, please indicate target tree (net) in [patch] brackets. >--- > 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 a3f100769e39..22fb2c322bca 100644 >--- a/drivers/net/ethernet/intel/igb/igb_main.c >+++ b/drivers/net/ethernet/intel/igb/igb_main.c >@@ -4833,6 +4833,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 >-- >2.44.0 > >
Tue, Apr 23, 2024 at 12:24:46PM CEST, vinschen@redhat.com 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. > >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_FRAG") >Reported-by: Jan Tluka <jtluka@redhat.com> >Reported-by: Jirka Hladky <jhladky@redhat.com> >Reported-by: Sabrina Dubroca <sd@queasysnail.net> >Tested-by: Sabrina Dubroca <sd@queasysnail.net> >Tested-by: Corinna Vinschen <vinschen@redhat.com> >Signed-off-by: Paolo Abeni <pabeni@redhat.com> >--- > drivers/net/ethernet/intel/igb/igb_main.c | 1 + Also, please use get_maintainer.pl script to get cclist.
Dear Corinna, Thank you for the patch. Am 23.04.24 um 12:24 schrieb Corinna Vinschen: > From: Paolo Abeni <pabeni@redhat.com> It’d be great if you removed the trailing dot/period in the commit message summary. > 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. > > 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. If you have a reproducer for this, it’d be great if you could document it in the commit message. > Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAG") > Reported-by: Jan Tluka <jtluka@redhat.com> > Reported-by: Jirka Hladky <jhladky@redhat.com> > Reported-by: Sabrina Dubroca <sd@queasysnail.net> > Tested-by: Sabrina Dubroca <sd@queasysnail.net> > Tested-by: Corinna Vinschen <vinschen@redhat.com> > Signed-off-by: Paolo Abeni <pabeni@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 a3f100769e39..22fb2c322bca 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -4833,6 +4833,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 Kind regards, Paul
Hi Paul, On Apr 23 13:52, Paul Menzel wrote: > Dear Corinna, > > > Thank you for the patch. > > > Am 23.04.24 um 12:24 schrieb Corinna Vinschen: > > From: Paolo Abeni <pabeni@redhat.com> > > It’d be great if you removed the trailing dot/period in the commit message > summary. > > > 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. > > > > 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. > > If you have a reproducer for this, it’d be great if you could document it in > the commit message. I fixed the trailing dot and added a reproducer. Thanks, Corinna
Hi Jiri, On Apr 23 13:26, Jiri Pirko wrote: > Tue, Apr 23, 2024 at 12:24:46PM CEST, vinschen@redhat.com 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. > > > >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_FRAG") > >Reported-by: Jan Tluka <jtluka@redhat.com> > >Reported-by: Jirka Hladky <jhladky@redhat.com> > >Reported-by: Sabrina Dubroca <sd@queasysnail.net> > >Tested-by: Sabrina Dubroca <sd@queasysnail.net> > >Tested-by: Corinna Vinschen <vinschen@redhat.com> > >Signed-off-by: Paolo Abeni <pabeni@redhat.com> > >--- > > drivers/net/ethernet/intel/igb/igb_main.c | 1 + > > Also, please use get_maintainer.pl script to get cclist. done and done in v2 (for which I forgot the "in-reply-to" now, d'uh) Thanks, Corinna
Tue, Apr 23, 2024 at 03:54:05PM CEST, vinschen@redhat.com wrote: >Hi Jiri, > >On Apr 23 13:26, Jiri Pirko wrote: >> Tue, Apr 23, 2024 at 12:24:46PM CEST, vinschen@redhat.com 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. >> > >> >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_FRAG") >> >Reported-by: Jan Tluka <jtluka@redhat.com> >> >Reported-by: Jirka Hladky <jhladky@redhat.com> >> >Reported-by: Sabrina Dubroca <sd@queasysnail.net> >> >Tested-by: Sabrina Dubroca <sd@queasysnail.net> >> >Tested-by: Corinna Vinschen <vinschen@redhat.com> >> >Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> >--- >> > drivers/net/ethernet/intel/igb/igb_main.c | 1 + >> >> Also, please use get_maintainer.pl script to get cclist. > >done and done in v2 (for which I forgot the "in-reply-to" now, d'uh) In-reply-to is not needed. Send each V to separate thread. > >Thanks, >Corinna > >
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index a3f100769e39..22fb2c322bca 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -4833,6 +4833,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