Re: [Xen-devel] [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to the guest

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 08 October 2013 14:34
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; David Vrabel;
> Ian Campbell
> Subject: Re: [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to
> the guest
> 
> On Tue, Oct 08, 2013 at 11:58:16AM +0100, Paul Durrant wrote:
> [...]
> >     /* Data must not cross a page boundary. */
> >     BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
> > @@ -392,7 +393,14 @@ static void xenvif_gop_frag_copy(struct xenvif
> *vif, struct sk_buff *skb,
> >             }
> >
> >             /* Leave a gap for the GSO descriptor. */
> > -           if (*head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
> > +           if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4)
> > +                   gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
> > +           else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
> > +                   gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
> > +           else
> > +                   gso_type = 0;
> 
> Should probably #define XEN_NETIF_GSO_TYPE_NONE 0 instead of using
> plain
> 0?

All I need is a bit shift that's not going to hit in the mask. Probably worth 
reserving the value in the header.

> 
> > +   if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
> > +           gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
> > +           gso_size = skb_shinfo(skb)->gso_size;
> > +   } else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
> > +           gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
> > +           gso_size = skb_shinfo(skb)->gso_size;
> > +   } else {
> > +           gso_type = 0;
> 
> Ditto.
> 
> > -   if (!vif->gso_prefix)
> > -           meta->gso_size = skb_shinfo(skb)->gso_size;
> > -   else
> > +   if ((1 << gso_type) & vif->gso_mask) {
> > +           meta->gso_type = gso_type;
> > +           meta->gso_size = gso_size;
> > +   } else {
> > +           meta->gso_type = 0;
> 
> Ditto.
> 
> >             meta->gso_size = 0;
> [...]
> > @@ -573,15 +573,40 @@ static int connect_rings(struct backend_info *be)
> >             val = 0;
> >     vif->can_sg = !!val;
> >
> > +   vif->gso_mask = 0;
> > +   vif->gso_prefix_mask = 0;
> > +
> >     if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
> >                      "%d", &val) < 0)
> >             val = 0;
> > -   vif->gso = !!val;
> > +   if (val)
> > +           vif->gso_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV4;
> 
> Not using "|=" ?
> 
> >
> >     if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-
> prefix",
> >                      "%d", &val) < 0)
> >             val = 0;
> > -   vif->gso_prefix = !!val;
> > +   if (val)
> > +           vif->gso_prefix_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV4;
> > +
> > +   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6",
> > +                    "%d", &val) < 0)
> > +           val = 0;
> > +   if (val)
> > +           vif->gso_mask = 1 << XEN_NETIF_GSO_TYPE_TCPV6;
> > +
> 
> Not using "|="? Are feature-gso_tcpv{4,6} mutually exclusive?
> 
> Same question goes to the setting of gso_prefix_mask.
> 

That's very odd. You're correct and I could have sworn the code did have |= in 
these places; thanks for catching that.

  Paul

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