| Message ID | 1415138202-1197-4-git-send-email-joestringer@nicira.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote: > ndo_gso_check() was recently introduced to allow NICs to report the > offloading support that they have on a per-skb basis. Add an > implementation for this driver which checks for something that looks > like VXLAN. > > Implementation shamelessly stolen from Tom Herbert: > http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > Should this driver report support for GSO on packets with tunnel > headers > up to 64B like the i40e driver does? > --- > drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Thanks Joe, I will add your patch to my queue.
On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote: >> ndo_gso_check() was recently introduced to allow NICs to report the >> offloading support that they have on a per-skb basis. Add an >> implementation for this driver which checks for something that looks >> like VXLAN. >> >> Implementation shamelessly stolen from Tom Herbert: >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111 >> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> --- >> Should this driver report support for GSO on packets with tunnel >> headers >> up to 64B like the i40e driver does? >> --- >> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) > > Thanks Joe, I will add your patch to my queue. Hi Jeff, please see my comment on patch 0/5, we're essentially replicating the same helper four different times (fm10k, mlx4, benet, qlgc) - I don't see the point in doing so. I asked Joe to come up with one generic helper and then to pick it up by the four drivers, makes sense? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote: > On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher > <jeffrey.t.kirsher@intel.com> wrote: > > On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote: > >> ndo_gso_check() was recently introduced to allow NICs to report the > >> offloading support that they have on a per-skb basis. Add an > >> implementation for this driver which checks for something that looks > >> like VXLAN. > >> > >> Implementation shamelessly stolen from Tom Herbert: > >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > >> > >> Signed-off-by: Joe Stringer <joestringer@nicira.com> > >> --- > >> Should this driver report support for GSO on packets with tunnel > >> headers > >> up to 64B like the i40e driver does? > >> --- > >> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > >> 1 file changed, 12 insertions(+) > > > > Thanks Joe, I will add your patch to my queue. > > Hi Jeff, please see my comment on patch 0/5, we're essentially > replicating the same helper four different times (fm10k, mlx4, benet, > qlgc) - I don't see the point in doing so. I asked Joe to come up with > one generic helper and then to pick it up by the four drivers, makes > sense? Yeah, I just saw your reply Or. Ok, I will await an update to Joe's series, thanks!
On Wed, 2014-11-05 at 10:26 -0800, Joe Stringer wrote: > On 5 November 2014 04:47, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > > > > On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote: > > > On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher > > > <jeffrey.t.kirsher@intel.com> wrote: > > > > On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote: > > > >> ndo_gso_check() was recently introduced to allow NICs to report the > > > >> offloading support that they have on a per-skb basis. Add an > > > >> implementation for this driver which checks for something that looks > > > >> like VXLAN. > > > >> > > > >> Implementation shamelessly stolen from Tom Herbert: > > > >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > > > >> > > > >> Signed-off-by: Joe Stringer <joestringer@nicira.com> > > > >> --- > > > >> Should this driver report support for GSO on packets with tunnel > > > >> headers > > > >> up to 64B like the i40e driver does? > > > >> --- > > > >> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > > > >> 1 file changed, 12 insertions(+) > > > > > > > > Thanks Joe, I will add your patch to my queue. > > > > > > Hi Jeff, please see my comment on patch 0/5, we're essentially > > > replicating the same helper four different times (fm10k, mlx4, benet, > > > qlgc) - I don't see the point in doing so. I asked Joe to come up with > > > one generic helper and then to pick it up by the four drivers, makes > > > sense? > > > > Yeah, I just saw your reply Or. Ok, I will await an update to Joe's > > series, thanks! > > Thanks Or/Jeff. > > There is also the question in the commit message above, perhaps fm10k > support is a bit different - wasn't sure who to ask regarding that. Matthew Vick is the fm10k maintainer now and can answer any fm10k questions you may have.
On 11/04/2014 01:56 PM, Joe Stringer wrote: > ndo_gso_check() was recently introduced to allow NICs to report the > offloading support that they have on a per-skb basis. Add an > implementation for this driver which checks for something that looks > like VXLAN. > > Implementation shamelessly stolen from Tom Herbert: > http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > Should this driver report support for GSO on packets with tunnel headers > up to 64B like the i40e driver does? > --- > drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > index 8811364..b9ef622 100644 > --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv) > } > } > > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > +{ > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) && > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER || > + skb->inner_protocol != htons(ETH_P_TEB) || > + skb_inner_mac_header(skb) - skb_transport_header(skb) != 16)) > + return false; > + > + return true; > +} > + > static const struct net_device_ops fm10k_netdev_ops = { > .ndo_open = fm10k_open, > .ndo_stop = fm10k_close, > @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = { > .ndo_do_ioctl = fm10k_ioctl, > .ndo_dfwd_add_station = fm10k_dfwd_add_station, > .ndo_dfwd_del_station = fm10k_dfwd_del_station, > + .ndo_gso_check = fm10k_gso_check, > }; > > #define DEFAULT_DEBUG_LEVEL_SHIFT 3 I'm thinking this check is far too simplistic. If you look the fm10k driver already has fm10k_tx_encap_offload() in the TSO function for verifying if it can support offloading tunnels or not. I would recommend starting there or possibly even just adapting that function to suit your purpose. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote: > On 11/04/2014 01:56 PM, Joe Stringer wrote: > > ndo_gso_check() was recently introduced to allow NICs to report the > > offloading support that they have on a per-skb basis. Add an > > implementation for this driver which checks for something that looks > > like VXLAN. > > > > Implementation shamelessly stolen from Tom Herbert: > > http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > > > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > > --- > > Should this driver report support for GSO on packets with tunnel headers > > up to 64B like the i40e driver does? > > --- > > drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > index 8811364..b9ef622 100644 > > --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv) > > } > > } > > > > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > > +{ > > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) && > > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER || > > + skb->inner_protocol != htons(ETH_P_TEB) || > > + skb_inner_mac_header(skb) - skb_transport_header(skb) != 16)) > > + return false; > > + > > + return true; > > +} > > + > > static const struct net_device_ops fm10k_netdev_ops = { > > .ndo_open = fm10k_open, > > .ndo_stop = fm10k_close, > > @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = { > > .ndo_do_ioctl = fm10k_ioctl, > > .ndo_dfwd_add_station = fm10k_dfwd_add_station, > > .ndo_dfwd_del_station = fm10k_dfwd_del_station, > > + .ndo_gso_check = fm10k_gso_check, > > }; > > > > #define DEFAULT_DEBUG_LEVEL_SHIFT 3 > > I'm thinking this check is far too simplistic. If you look the fm10k > driver already has fm10k_tx_encap_offload() in the TSO function for > verifying if it can support offloading tunnels or not. I would > recommend starting there or possibly even just adapting that function to > suit your purpose. > > Thanks, > > Alex Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()? +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) +{ + if (skb->encapsulation && !fm10k_tx_encap_offload(skb)) + return false; + + return true; +} Thanks, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2014 at 10:41:15AM -0800, Joe Stringer wrote: > On Wed, Nov 05, 2014 at 06:54:00PM -0800, Alexander Duyck wrote: > > On 11/04/2014 01:56 PM, Joe Stringer wrote: > > > ndo_gso_check() was recently introduced to allow NICs to report the > > > offloading support that they have on a per-skb basis. Add an > > > implementation for this driver which checks for something that looks > > > like VXLAN. > > > > > > Implementation shamelessly stolen from Tom Herbert: > > > http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > > > > > > Signed-off-by: Joe Stringer <joestringer@nicira.com> > > > --- > > > Should this driver report support for GSO on packets with tunnel headers > > > up to 64B like the i40e driver does? > > > --- > > > drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > > index 8811364..b9ef622 100644 > > > --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c > > > @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv) > > > } > > > } > > > > > > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > > > +{ > > > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) && > > > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER || > > > + skb->inner_protocol != htons(ETH_P_TEB) || > > > + skb_inner_mac_header(skb) - skb_transport_header(skb) != 16)) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > static const struct net_device_ops fm10k_netdev_ops = { > > > .ndo_open = fm10k_open, > > > .ndo_stop = fm10k_close, > > > @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = { > > > .ndo_do_ioctl = fm10k_ioctl, > > > .ndo_dfwd_add_station = fm10k_dfwd_add_station, > > > .ndo_dfwd_del_station = fm10k_dfwd_del_station, > > > + .ndo_gso_check = fm10k_gso_check, > > > }; > > > > > > #define DEFAULT_DEBUG_LEVEL_SHIFT 3 > > > > I'm thinking this check is far too simplistic. If you look the fm10k > > driver already has fm10k_tx_encap_offload() in the TSO function for > > verifying if it can support offloading tunnels or not. I would > > recommend starting there or possibly even just adapting that function to > > suit your purpose. > > > > Thanks, > > > > Alex > > Would it be enough to just call fm10k_tx_encap_offload() in a way that echoes fm10k_tso()? > > +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > +{ > + if (skb->encapsulation && !fm10k_tx_encap_offload(skb)) > + return false; > + > + return true; > +} Oh, I suppose we need to check the gso_type too. More like this? +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) +{ + if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) && + !fm10k_tx_encap_offload(skb)) + return false; + + return true; +} -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2014 at 11:58:32PM +0000, Vick, Matthew wrote: > On 11/5/14, 11:36 AM, "Jeff Kirsher" <jeffrey.t.kirsher@intel.com> wrote: > > >On Wed, 2014-11-05 at 10:26 -0800, Joe Stringer wrote: > >> On 5 November 2014 04:47, Jeff Kirsher <jeffrey.t.kirsher@intel.com> > >>wrote: > >> > > >> > On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote: > >> > > On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher > >> > > <jeffrey.t.kirsher@intel.com> wrote: > >> > > > On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote: > >> > > >> ndo_gso_check() was recently introduced to allow NICs to report > >>the > >> > > >> offloading support that they have on a per-skb basis. Add an > >> > > >> implementation for this driver which checks for something that > >>looks > >> > > >> like VXLAN. > >> > > >> > >> > > >> Implementation shamelessly stolen from Tom Herbert: > >> > > >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111 > >> > > >> > >> > > >> Signed-off-by: Joe Stringer <joestringer@nicira.com> > >> > > >> --- > >> > > >> Should this driver report support for GSO on packets with tunnel > >> > > >> headers > >> > > >> up to 64B like the i40e driver does? > >> > > >> --- > >> > > >> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 > >>++++++++++++ > >> > > >> 1 file changed, 12 insertions(+) > >> > > > > >> > > > Thanks Joe, I will add your patch to my queue. > >> > > > >> > > Hi Jeff, please see my comment on patch 0/5, we're essentially > >> > > replicating the same helper four different times (fm10k, mlx4, > >>benet, > >> > > qlgc) - I don't see the point in doing so. I asked Joe to come up > >>with > >> > > one generic helper and then to pick it up by the four drivers, makes > >> > > sense? > >> > > >> > Yeah, I just saw your reply Or. Ok, I will await an update to Joe's > >> > series, thanks! > >> > >> Thanks Or/Jeff. > >> > >> There is also the question in the commit message above, perhaps fm10k > >> support is a bit different - wasn't sure who to ask regarding that. > > > >Matthew Vick is the fm10k maintainer now and can answer any fm10k > >questions you may have. > > Hi Joe, fm10k's hardware is pretty lax about the header size. As long as > the total header length (outer+inner) is 184 bytes or less we're golden, > so if I'm not mistaken that leaves us with a max of 130 bytes beyond the > tunnel header. Oh, okay. To be more explicit, in the case of UDP tunnels I take it that you're talking about L2+L3+(L4+)tunnel+L2+L3+L4 <= 184? (L4 perhaps optional depending on the tunnel protocol used) In that case, the fm10k_gso_check would use something closer to "skb_inner_transport_header(skb) - skb_mac_header(skb) > 184", or perhaps 164 to allow for inner L4 header (?). Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/6/14, 1:15 PM, "Joe Stringer" <joestringer@nicira.com> wrote: >Oh, I suppose we need to check the gso_type too. More like this? > >+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) >+{ >+ if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | >SKB_GSO_GRE)) && >+ !fm10k_tx_encap_offload(skb)) >+ return false; >+ >+ return true; >+} It seems like the skb_shinfo(skb)->gso_type check should be in some more generic ndo_gso_check that drivers can default to/extend. Then, we could end up with something like static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) { if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb)) return false; return true; } This could even be simplified and still legible as static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) { return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb)); } What do you think of this approach? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 07 Nov 2014 14:20:08 Vick, Matthew wrote: > On 11/6/14, 4:55 PM, "Joe Stringer" <joestringer@nicira.com> wrote: > >On Thu, Nov 06, 2014 at 11:58:32PM +0000, Vick, Matthew wrote: > >> On 11/5/14, 11:36 AM, "Jeff Kirsher" <jeffrey.t.kirsher@intel.com> > >> > >>wrote: > >> Hi Joe, fm10k's hardware is pretty lax about the header size. As long as > >> the total header length (outer+inner) is 184 bytes or less we're golden, > >> so if I'm not mistaken that leaves us with a max of 130 bytes beyond the > >> tunnel header. > > > >Oh, okay. To be more explicit, in the case of UDP tunnels I take it that > >you're talking about L2+L3+(L4+)tunnel+L2+L3+L4 <= 184? (L4 perhaps > >optional depending on the tunnel protocol used) > > > >In that case, the fm10k_gso_check would use something closer to > >"skb_inner_transport_header(skb) - skb_mac_header(skb) > 184", or > >perhaps 164 to allow for inner L4 header (?). > > > >Joe > > Yes, I'm talking about the full shebang. > > I like the 164 check, personally (with appropriate #define for > readability). Thanks for the feedback, I take it that this approach is preferable over the other one involving an skb_gso_check() + fm10k_tx_encap_offload() call? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 07 Nov 2014 14:07:36 Vick, Matthew wrote: > On 11/6/14, 1:15 PM, "Joe Stringer" <joestringer@nicira.com> wrote: > >Oh, I suppose we need to check the gso_type too. More like this? > > > >+static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > >+{ > >+ if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | > >SKB_GSO_GRE)) && > >+ !fm10k_tx_encap_offload(skb)) > >+ return false; > >+ > >+ return true; > >+} > > It seems like the skb_shinfo(skb)->gso_type check should be in some more > generic ndo_gso_check that drivers can default to/extend. Then, we could > end up with something like > > static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > { > if (skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb)) > return false; > > return true; > } > > This could even be simplified and still legible as > > static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > { > return !(skb_gso_check(skb, dev) && !fm10k_tx_encap_offload(skb)); > } > > What do you think of this approach? Let's merge both discussions into one thread (pick here or there). We have this suggestion or the one which simply checks for tunnels and inner+outer header lengths. Do you have a preference between them? We could introduce an "skb_is_gso_encap()" or similar for this purpose. Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to what fm10k_tx_encap_offload() checks for though; it might not even make sense to call it if some of the other SKB_GSO_* flags are raised. Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/6/14, 9:05 PM, "Joe Stringer" <joestringer@nicira.com> wrote: >Let's merge both discussions into one thread (pick here or there). We >have >this suggestion or the one which simply checks for tunnels and >inner+outer >header lengths. Do you have a preference between them? Agreed with merging the thread--consider it merged! Reflecting on this some more, I prefer the latter option (checking tunnels and header lengths). I'm leaning towards pushing the header length check into fm10k_tx_encap_offload and then making fm10k_gso_check call that with the gso_type. So, it's really the most recent version of the patch you proposed: static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) { if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) && !fm10k_tx_encap_offload(skb)) return false; return true; } plus the header length being checked in fm10k_tx_encap_offload. The only nit would be that I'd just return the conditional instead of having "return true" or "return false" lines. The tunnel length check really should be there in fm10k_tx_encap_offload anyway, so I'll get a patch together for that one. >We could introduce an "skb_is_gso_encap()" or similar for this purpose. >Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to >what >fm10k_tx_encap_offload() checks for though; it might not even make sense >to call >it if some of the other SKB_GSO_* flags are raised. A fair point. On the other hand, we have to check the header length both in the GSO and non-GSO cases anyway, so only having the check in fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as duplicative. What do you think about that approach? As an aside: the more I think about this, the more I think Tom's right and that each driver really should have it's own ndo_gso_check() for this. With fm10k and i40e being different, we're already at 40% of the current drivers being different. I'll leave it to Or to comment on whether the other drivers could share the check in some manner. Cheers, Matthew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote: > On 11/6/14, 9:05 PM, "Joe Stringer" <joestringer@nicira.com> wrote: > >Let's merge both discussions into one thread (pick here or there). We > >have > >this suggestion or the one which simply checks for tunnels and > >inner+outer > >header lengths. Do you have a preference between them? > > Agreed with merging the thread--consider it merged! > > Reflecting on this some more, I prefer the latter option (checking tunnels > and header lengths). I'm leaning towards pushing the header length check > into fm10k_tx_encap_offload and then making fm10k_gso_check call that with > the gso_type. So, it's really the most recent version of the patch you > proposed: > > static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > { > if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) && > !fm10k_tx_encap_offload(skb)) > return false; > > return true; > } > > > plus the header length being checked in fm10k_tx_encap_offload. The only > nit would be that I'd just return the conditional instead of having > "return true" or "return false" lines. OK, that sounds reasonable. > The tunnel length check really should be there in fm10k_tx_encap_offload > anyway, so I'll get a patch together for that one. Thanks. > >We could introduce an "skb_is_gso_encap()" or similar for this purpose. > >Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to > >what > >fm10k_tx_encap_offload() checks for though; it might not even make sense > >to call > >it if some of the other SKB_GSO_* flags are raised. > > A fair point. On the other hand, we have to check the header length both > in the GSO and non-GSO cases anyway, so only having the check in > fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as > duplicative. What do you think about that approach? Sure, I think fm10k_tx_encap_offload() is a good place for the header length check. Separately, my question above was regarding the idea of a helper for SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the fm10k driver is because all encap is checked in the fm10k_tx_encap_offload() function. Other drivers don't seem to handle different tunnels together like this though, so I'm inclined to stick with the below for now. static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) { return (!(skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) || fm10k_tx_encap_offload(skb)); } Cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/7/14, 2:35 PM, "Joe Stringer" <joestringer@nicira.com> wrote: >Sure, I think fm10k_tx_encap_offload() is a good place for the header >length >check. Separately, my question above was regarding the idea of a helper >for >SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the >fm10k >driver is because all encap is checked in the fm10k_tx_encap_offload() >function. >Other drivers don't seem to handle different tunnels together like this >though, >so I'm inclined to stick with the below for now. > > >static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) > >{ > > return (!(skb_shinfo(skb)->gso_type & > > (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) || > > fm10k_tx_encap_offload(skb)); > >} > >Cheers, >Joe I agree. I think that makes the most sense. Cheers, Matthew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c index 8811364..b9ef622 100644 --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c @@ -1350,6 +1350,17 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv) } } +static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev) +{ + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) && + (skb->inner_protocol_type != ENCAP_TYPE_ETHER || + skb->inner_protocol != htons(ETH_P_TEB) || + skb_inner_mac_header(skb) - skb_transport_header(skb) != 16)) + return false; + + return true; +} + static const struct net_device_ops fm10k_netdev_ops = { .ndo_open = fm10k_open, .ndo_stop = fm10k_close, @@ -1372,6 +1383,7 @@ static const struct net_device_ops fm10k_netdev_ops = { .ndo_do_ioctl = fm10k_ioctl, .ndo_dfwd_add_station = fm10k_dfwd_add_station, .ndo_dfwd_del_station = fm10k_dfwd_del_station, + .ndo_gso_check = fm10k_gso_check, }; #define DEFAULT_DEBUG_LEVEL_SHIFT 3
ndo_gso_check() was recently introduced to allow NICs to report the offloading support that they have on a per-skb basis. Add an implementation for this driver which checks for something that looks like VXLAN. Implementation shamelessly stolen from Tom Herbert: http://thread.gmane.org/gmane.linux.network/332428/focus=333111 Signed-off-by: Joe Stringer <joestringer@nicira.com> --- Should this driver report support for GSO on packets with tunnel headers up to 64B like the i40e driver does? --- drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)