Re: [Xen-devel] [PATCH net-next v2] xen-netfront: Add support for IPv6 offloads

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: annie li [mailto:annie.li@xxxxxxxxxx]
> Sent: 26 November 2013 01:58
> To: Konrad Rzeszutek Wilk
> Cc: Paul Durrant; Wei Liu; Boris Ostrovsky; David Vrabel; Ian Campbell; xen-
> devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH net-next v2] xen-netfront: Add support for
> IPv6 offloads
> 
> 
> On 2013/11/25 23:19, Konrad Rzeszutek Wilk wrote:
> > On Fri, Nov 15, 2013 at 05:52:59PM +0000, Paul Durrant wrote:
> >> This patch adds support for IPv6 checksum offload and GSO when those
> >> features are available in the backend.
> > Wei, Annie, thoughts?
> 
> Seems fine except for following two comments :-)
> 
> >> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> >> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >> ---
> >>
> >> v2:
> >>   - Addressed comments raised by Ian Campbell
> >>
> >>   drivers/net/xen-netfront.c |  226
> ++++++++++++++++++++++++++++++++++++++++----
> >>   include/linux/ipv6.h       |    2 +
> >>   2 files changed, 211 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> >> index dd1011e..afadfb5 100644
> >> --- a/drivers/net/xen-netfront.c
> >> +++ b/drivers/net/xen-netfront.c
> >> @@ -616,7 +616,9 @@ static int xennet_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> >>            tx->flags |= XEN_NETTXF_extra_info;
> >>
> >>            gso->u.gso.size = skb_shinfo(skb)->gso_size;
> >> -          gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4;
> >> +          gso->u.gso.type = (skb_shinfo(skb)->gso_type &
> SKB_GSO_TCPV6) ?
> >> +                            XEN_NETIF_GSO_TYPE_TCPV6 :
> >> +                            XEN_NETIF_GSO_TYPE_TCPV4;
> >>            gso->u.gso.pad = 0;
> >>            gso->u.gso.features = 0;
> 
> ...snip...
> 
> >>   static int handle_incoming_queue(struct net_device *dev,
> >>                             struct sk_buff_head *rxq)
> >>   {
> >> @@ -1232,6 +1392,15 @@ static netdev_features_t
> xennet_fix_features(struct net_device *dev,
> >>                    features &= ~NETIF_F_SG;
> >>    }
> >>
> >> +  if (features & NETIF_F_IPV6_CSUM) {
> >> +          if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
> >> +                           "feature-ipv6-csum-offload", "%d", &val) <
> 0)
> >> +                  val = 0;
> >> +
> >> +          if (!val)
> >> +                  features &= ~NETIF_F_IPV6_CSUM;
> >> +  }
> >> +
> >>    if (features & NETIF_F_TSO) {
> >>            if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
> >>                             "feature-gso-tcpv4", "%d", &val) < 0)
> >> @@ -1241,6 +1410,15 @@ static netdev_features_t
> xennet_fix_features(struct net_device *dev,
> >>                    features &= ~NETIF_F_TSO;
> >>    }
> >>
> >> +  if (features & NETIF_F_TSO6) {
> >> +          if (xenbus_scanf(XBT_NIL, np->xbdev->otherend,
> >> +                           "feature-gso-tcpv6", "%d", &val) < 0)
> >> +                  val = 0;
> >> +
> >> +          if (!val)
> >> +                  features &= ~NETIF_F_TSO6;
> >> +  }
> >> +
> >>    return features;
> >>   }
> >>
> >> @@ -1373,7 +1551,9 @@ static struct net_device
> *xennet_create_dev(struct xenbus_device *dev)
> >>    netif_napi_add(netdev, &np->napi, xennet_poll, 64);
> >>    netdev->features        = NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
> >>                              NETIF_F_GSO_ROBUST;
> >> -  netdev->hw_features     = NETIF_F_IP_CSUM | NETIF_F_SG |
> NETIF_F_TSO;
> >> +  netdev->hw_features     = NETIF_F_SG |
> >> +                            NETIF_F_IPV6_CSUM |
> 
> Are you missing NETIF_F_IP_CSUM here?
> 

Unfortunately the context didn't quite stretch that far, but just below there 
is a line which ORs netdev->features and netdev->hw_features together so that 
fact that NETIF_F_IP_CSUM is in netdev->features is sufficient and its presence 
in hw_features seems incorrect since hw_features is the set that can be 
modified by xenstore negotiation and, while it's possible to disable checksum 
by negotiation, netfront doesn't do that.

> >> +                            NETIF_F_TSO | NETIF_F_TSO6;
> >>
> >>    /*
> >>            * Assume that all hw features are available for now. This set
> >> @@ -1751,6 +1931,18 @@ again:
> >>            goto abort_transaction;
> >>    }
> >>
> >> +  err = xenbus_printf(xbt, dev->nodename, "feature-gso-tcpv6",
> "%d", 1);
> >> +  if (err) {
> >> +          message = "writing feature-gso-tcpv6";
> >> +          goto abort_transaction;
> >> +  }
> >> +
> >> +  err = xenbus_printf(xbt, dev->nodename, "feature-ipv6-csum-
> offload", "%d", 1);
> >> +  if (err) {
> >> +          message = "writing feature-gso-tcpv6";
> 
> Change "feature-gso-tcpv6" to "feature-ipv6-csum-offload"?
> 

Good catch! Thanks,

  Paul

> Thanks
> Annie

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel