diff mbox

[net-next,v2,2/2] ip_gre: propogate target device GSO capability to the tunnel device

Message ID 1361485805.21950.6.camel@lb-tlvb-dmitry
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Dmitry Kravkov Feb. 21, 2013, 10:30 p.m. UTC 
On Tue, 2013-02-19 at 10:39 -0800, pravin wrote:
> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> >
> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> > ---
> > Changes from v1: fixed email address
> >
> >
> >  net/ipv4/ip_gre.c |   10 ++++++++--
> >  1 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > index cdc31ac..31bc941 100644
> > --- a/net/ipv4/ip_gre.c
> > +++ b/net/ipv4/ip_gre.c
> > @@ -1103,8 +1103,14 @@ static int ipgre_tunnel_bind_dev(struct net_device *dev)
> >         tunnel->hlen = addend;
> >         /* TCP offload with GRE SEQ is not supported. */
> >         if (!(tunnel->parms.o_flags & GRE_SEQ)) {
> > -               dev->features           |= NETIF_F_GSO_SOFTWARE;
> > -               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> > +               /* device supports enc gso offload*/
> > +               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
> > +                       dev->features           |= NETIF_F_TSO;
> > +                       dev->hw_features        |= NETIF_F_TSO;
> > +               } else {
> > +                       dev->features           |= NETIF_F_GSO_SOFTWARE;
> > +                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> > +               }
> >         }
> 
> I am not sure about this change, Are you trying to limit GRE TSO to
> just IPV4  in case of GRE offload in hardware?


This mostly reverts previous patch, but also disables GSO when CSUM is
set.

---
 net/ipv4/ip_gre.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

 	return mtu;

Comments

pravin Feb. 21, 2013, 11:53 p.m. UTC | #1 
On Thu, Feb 21, 2013 at 2:30 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> On Tue, 2013-02-19 at 10:39 -0800, pravin wrote:
>> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
>> >
>> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
>> > ---
>> > Changes from v1: fixed email address
>> >
>> >
>> >  net/ipv4/ip_gre.c |   10 ++++++++--
>> >  1 files changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> > index cdc31ac..31bc941 100644
>> > --- a/net/ipv4/ip_gre.c
>> > +++ b/net/ipv4/ip_gre.c
>> > @@ -1103,8 +1103,14 @@ static int ipgre_tunnel_bind_dev(struct net_device *dev)
>> >         tunnel->hlen = addend;
>> >         /* TCP offload with GRE SEQ is not supported. */
>> >         if (!(tunnel->parms.o_flags & GRE_SEQ)) {
>> > -               dev->features           |= NETIF_F_GSO_SOFTWARE;
>> > -               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> > +               /* device supports enc gso offload*/
>> > +               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
>> > +                       dev->features           |= NETIF_F_TSO;
>> > +                       dev->hw_features        |= NETIF_F_TSO;
>> > +               } else {
>> > +                       dev->features           |= NETIF_F_GSO_SOFTWARE;
>> > +                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> > +               }
>> >         }
>>
>> I am not sure about this change, Are you trying to limit GRE TSO to
>> just IPV4  in case of GRE offload in hardware?
>
>
> This mostly reverts previous patch, but also disables GSO when CSUM is
> set.
>
> ---
>  net/ipv4/ip_gre.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 0de54cd..aae2d4b 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1107,16 +1107,10 @@ static int ipgre_tunnel_bind_dev(struct
> net_device *dev)
>                 mtu = 68;
>
>         tunnel->hlen = addend;
> -       /* TCP offload with GRE SEQ is not supported. */
> -       if (!(tunnel->parms.o_flags & GRE_SEQ)) {
> -               /* device supports enc gso offload*/
> -               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
> -                       dev->features           |= NETIF_F_TSO;
> -                       dev->hw_features        |= NETIF_F_TSO;
> -               } else {
> -                       dev->features           |= NETIF_F_GSO_SOFTWARE;
> -                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> -               }
> +       /* TCP offload with GRE SEQ or CSUM is not supported. */
> +       if (!(tunnel->parms.o_flags & (GRE_SEQ|GRE_CSUM))) {
> +               dev->features           |= NETIF_F_GSO_SOFTWARE;
> +               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>         }
>
>         return mtu;

This narrows down definition of NETIF_F_GRE_GSO. Is it required for
hardware offload?

> --
> 1.7.7.2
>
>
>> >
>> >         return mtu;
>> > --
>> > 1.7.7.2
>> >
>> >
>> > --
>> > 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
>>
>
>
>
--
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
Dmitry Kravkov Feb. 24, 2013, 4:45 p.m. UTC | #2 
On Thu, 2013-02-21 at 15:53 -0800, pravin wrote:
> On Thu, Feb 21, 2013 at 2:30 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> > On Tue, 2013-02-19 at 10:39 -0800, pravin wrote:
> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> >> >
> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> >> > ---
> >> > Changes from v1: fixed email address
> >> >
> >> >
> >> >  net/ipv4/ip_gre.c |   10 ++++++++--
> >> >  1 files changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> >> > index cdc31ac..31bc941 100644
> >> > --- a/net/ipv4/ip_gre.c
> >> > +++ b/net/ipv4/ip_gre.c
> >> > @@ -1103,8 +1103,14 @@ static int ipgre_tunnel_bind_dev(struct net_device *dev)
> >> >         tunnel->hlen = addend;
> >> >         /* TCP offload with GRE SEQ is not supported. */
> >> >         if (!(tunnel->parms.o_flags & GRE_SEQ)) {
> >> > -               dev->features           |= NETIF_F_GSO_SOFTWARE;
> >> > -               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> >> > +               /* device supports enc gso offload*/
> >> > +               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
> >> > +                       dev->features           |= NETIF_F_TSO;
> >> > +                       dev->hw_features        |= NETIF_F_TSO;
> >> > +               } else {
> >> > +                       dev->features           |= NETIF_F_GSO_SOFTWARE;
> >> > +                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> >> > +               }
> >> >         }
> >>
> >> I am not sure about this change, Are you trying to limit GRE TSO to
> >> just IPV4  in case of GRE offload in hardware?
> >
> >
> > This mostly reverts previous patch, but also disables GSO when CSUM is
> > set.
> >
> > ---
> >  net/ipv4/ip_gre.c |   14 ++++----------
> >  1 files changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > index 0de54cd..aae2d4b 100644
> > --- a/net/ipv4/ip_gre.c
> > +++ b/net/ipv4/ip_gre.c
> > @@ -1107,16 +1107,10 @@ static int ipgre_tunnel_bind_dev(struct
> > net_device *dev)
> >                 mtu = 68;
> >
> >         tunnel->hlen = addend;
> > -       /* TCP offload with GRE SEQ is not supported. */
> > -       if (!(tunnel->parms.o_flags & GRE_SEQ)) {
> > -               /* device supports enc gso offload*/
> > -               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
> > -                       dev->features           |= NETIF_F_TSO;
> > -                       dev->hw_features        |= NETIF_F_TSO;
> > -               } else {
> > -                       dev->features           |= NETIF_F_GSO_SOFTWARE;
> > -                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> > -               }
> > +       /* TCP offload with GRE SEQ or CSUM is not supported. */
> > +       if (!(tunnel->parms.o_flags & (GRE_SEQ|GRE_CSUM))) {
> > +               dev->features           |= NETIF_F_GSO_SOFTWARE;
> > +               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
> >         }
> >
> >         return mtu;
> 
> This narrows down definition of NETIF_F_GRE_GSO. Is it required for
> hardware offload?

This is required to prevent dev_hard_xmit() to perform soft GSO.
W/o it device receives divided packets.




--
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
Pravin B Shelar Feb. 25, 2013, 6:07 a.m. UTC | #3 
On Sun, Feb 24, 2013 at 8:45 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> On Thu, 2013-02-21 at 15:53 -0800, pravin wrote:
>> On Thu, Feb 21, 2013 at 2:30 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
>> > On Tue, 2013-02-19 at 10:39 -0800, pravin wrote:
>> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
>> >> >
>> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
>> >> > ---
>> >> > Changes from v1: fixed email address
>> >> >
>> >> >
>> >> >  net/ipv4/ip_gre.c |   10 ++++++++--
>> >> >  1 files changed, 8 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> >> > index cdc31ac..31bc941 100644
>> >> > --- a/net/ipv4/ip_gre.c
>> >> > +++ b/net/ipv4/ip_gre.c
>> >> > @@ -1103,8 +1103,14 @@ static int ipgre_tunnel_bind_dev(struct net_device *dev)
>> >> >         tunnel->hlen = addend;
>> >> >         /* TCP offload with GRE SEQ is not supported. */
>> >> >         if (!(tunnel->parms.o_flags & GRE_SEQ)) {
>> >> > -               dev->features           |= NETIF_F_GSO_SOFTWARE;
>> >> > -               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> >> > +               /* device supports enc gso offload*/
>> >> > +               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
>> >> > +                       dev->features           |= NETIF_F_TSO;
>> >> > +                       dev->hw_features        |= NETIF_F_TSO;
>> >> > +               } else {
>> >> > +                       dev->features           |= NETIF_F_GSO_SOFTWARE;
>> >> > +                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> >> > +               }
>> >> >         }
>> >>
>> >> I am not sure about this change, Are you trying to limit GRE TSO to
>> >> just IPV4  in case of GRE offload in hardware?
>> >
>> >
>> > This mostly reverts previous patch, but also disables GSO when CSUM is
>> > set.
>> >
>> > ---
>> >  net/ipv4/ip_gre.c |   14 ++++----------
>> >  1 files changed, 4 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> > index 0de54cd..aae2d4b 100644
>> > --- a/net/ipv4/ip_gre.c
>> > +++ b/net/ipv4/ip_gre.c
>> > @@ -1107,16 +1107,10 @@ static int ipgre_tunnel_bind_dev(struct
>> > net_device *dev)
>> >                 mtu = 68;
>> >
>> >         tunnel->hlen = addend;
>> > -       /* TCP offload with GRE SEQ is not supported. */
>> > -       if (!(tunnel->parms.o_flags & GRE_SEQ)) {
>> > -               /* device supports enc gso offload*/
>> > -               if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
>> > -                       dev->features           |= NETIF_F_TSO;
>> > -                       dev->hw_features        |= NETIF_F_TSO;
>> > -               } else {
>> > -                       dev->features           |= NETIF_F_GSO_SOFTWARE;
>> > -                       dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> > -               }
>> > +       /* TCP offload with GRE SEQ or CSUM is not supported. */
>> > +       if (!(tunnel->parms.o_flags & (GRE_SEQ|GRE_CSUM))) {
>> > +               dev->features           |= NETIF_F_GSO_SOFTWARE;
>> > +               dev->hw_features        |= NETIF_F_GSO_SOFTWARE;
>> >         }
>> >
>> >         return mtu;
>>
>> This narrows down definition of NETIF_F_GRE_GSO. Is it required for
>> hardware offload?
>
> This is required to prevent dev_hard_xmit() to perform soft GSO.
> W/o it device receives divided packets.
>
If NIC features are set correctly dev_hard_xmit() will not perform GSO.

>
>
>
> --
> 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
--
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 mbox

Patch

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 0de54cd..aae2d4b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1107,16 +1107,10 @@  static int ipgre_tunnel_bind_dev(struct
net_device *dev)
 		mtu = 68;
 
 	tunnel->hlen = addend;
-	/* TCP offload with GRE SEQ is not supported. */
-	if (!(tunnel->parms.o_flags & GRE_SEQ)) {
-		/* device supports enc gso offload*/
-		if (tdev->hw_enc_features & NETIF_F_GRE_GSO) {
-			dev->features		|= NETIF_F_TSO;
-			dev->hw_features	|= NETIF_F_TSO;
-		} else {
-			dev->features		|= NETIF_F_GSO_SOFTWARE;
-			dev->hw_features	|= NETIF_F_GSO_SOFTWARE;
-		}
+	/* TCP offload with GRE SEQ or CSUM is not supported. */
+	if (!(tunnel->parms.o_flags & (GRE_SEQ|GRE_CSUM))) {
+		dev->features		|= NETIF_F_GSO_SOFTWARE;
+		dev->hw_features	|= NETIF_F_GSO_SOFTWARE;
 	}