Paul Durrant
2013-Oct-11 15:06 UTC
[PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to the guest
This patch adds code to handle SKB_GSO_TCPV6 skbs and construct appropriate extra or prefix segments to pass the large packet to the frontend. New xenstore flags, feature-gso-tcpv6 and feature-gso-tcpv6-prefix, are sampled to determine if the frontend is capable of handling such packets. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: David Vrabel <david.vrabel@citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> --- drivers/net/xen-netback/common.h | 9 +++++-- drivers/net/xen-netback/interface.c | 6 +++-- drivers/net/xen-netback/netback.c | 48 +++++++++++++++++++++++++++-------- drivers/net/xen-netback/xenbus.c | 29 +++++++++++++++++++-- include/xen/interface/io/netif.h | 1 + 5 files changed, 77 insertions(+), 16 deletions(-) diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index b4a9a3c..55b8dec 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -87,9 +87,13 @@ struct pending_tx_info { struct xenvif_rx_meta { int id; int size; + int gso_type; int gso_size; }; +#define GSO_BIT(type) \ + (1 << XEN_NETIF_GSO_TYPE_ ## type) + /* Discriminate from any valid pending_idx value. */ #define INVALID_PENDING_IDX 0xFFFF @@ -150,9 +154,10 @@ struct xenvif { u8 fe_dev_addr[6]; /* Frontend feature information. */ + int gso_mask; + int gso_prefix_mask; + u8 can_sg:1; - u8 gso:1; - u8 gso_prefix:1; u8 ip_csum:1; u8 ipv6_csum:1; diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index cb0d8ea..e4aa267 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -214,8 +214,10 @@ static netdev_features_t xenvif_fix_features(struct net_device *dev, if (!vif->can_sg) features &= ~NETIF_F_SG; - if (!vif->gso && !vif->gso_prefix) + if (~(vif->gso_mask | vif->gso_prefix_mask) & GSO_BIT(TCPV4)) features &= ~NETIF_F_TSO; + if (~(vif->gso_mask | vif->gso_prefix_mask) & GSO_BIT(TCPV6)) + features &= ~NETIF_F_TSO6; if (!vif->ip_csum) features &= ~NETIF_F_IP_CSUM; if (!vif->ipv6_csum) @@ -320,7 +322,7 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, dev->netdev_ops = &xenvif_netdev_ops; dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | - NETIF_F_TSO; + NETIF_F_TSO | NETIF_F_TSO6; dev->features = dev->hw_features | NETIF_F_RXCSUM; SET_ETHTOOL_OPS(dev, &xenvif_ethtool_ops); diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 65f04e7..b19d407 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -140,7 +140,7 @@ static int max_required_rx_slots(struct xenvif *vif) int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE); /* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */ - if (vif->can_sg || vif->gso || vif->gso_prefix) + if (vif->can_sg || vif->gso_mask || vif->gso_prefix_mask) max += MAX_SKB_FRAGS + 1; /* extra_info + frags */ return max; @@ -312,6 +312,7 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct xenvif *vif, req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++); meta = npo->meta + npo->meta_prod++; + meta->gso_type = XEN_NETIF_GSO_TYPE_NONE; meta->gso_size = 0; meta->size = 0; meta->id = req->id; @@ -334,6 +335,7 @@ static void xenvif_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb, struct gnttab_copy *copy_gop; struct xenvif_rx_meta *meta; unsigned long bytes; + int gso_type; /* Data must not cross a page boundary. */ BUG_ON(size + offset > PAGE_SIZE<<compound_order(page)); @@ -392,7 +394,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 = XEN_NETIF_GSO_TYPE_NONE; + + if (*head && ((1 << gso_type) & vif->gso_mask)) vif->rx.req_cons++; *head = 0; /* There must be something in this buffer now. */ @@ -423,14 +432,28 @@ static int xenvif_gop_skb(struct sk_buff *skb, unsigned char *data; int head = 1; int old_meta_prod; + int gso_type; + int gso_size; old_meta_prod = npo->meta_prod; + 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 = XEN_NETIF_GSO_TYPE_NONE; + gso_size = 0; + } + /* Set up a GSO prefix descriptor, if necessary */ - if (skb_shinfo(skb)->gso_size && vif->gso_prefix) { + if ((1 << skb_shinfo(skb)->gso_type) & vif->gso_prefix_mask) { req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++); meta = npo->meta + npo->meta_prod++; - meta->gso_size = skb_shinfo(skb)->gso_size; + meta->gso_type = gso_type; + meta->gso_size = gso_size; meta->size = 0; meta->id = req->id; } @@ -438,10 +461,13 @@ static int xenvif_gop_skb(struct sk_buff *skb, req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++); meta = npo->meta + npo->meta_prod++; - 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 = XEN_NETIF_GSO_TYPE_NONE; meta->gso_size = 0; + } meta->size = 0; meta->id = req->id; @@ -587,7 +613,8 @@ void xenvif_rx_action(struct xenvif *vif) vif = netdev_priv(skb->dev); - if (vif->meta[npo.meta_cons].gso_size && vif->gso_prefix) { + if ((1 << vif->meta[npo.meta_cons].gso_type) & + vif->gso_prefix_mask) { resp = RING_GET_RESPONSE(&vif->rx, vif->rx.rsp_prod_pvt++); @@ -624,7 +651,8 @@ void xenvif_rx_action(struct xenvif *vif) vif->meta[npo.meta_cons].size, flags); - if (vif->meta[npo.meta_cons].gso_size && !vif->gso_prefix) { + if ((1 << vif->meta[npo.meta_cons].gso_type) & + vif->gso_mask) { struct xen_netif_extra_info *gso (struct xen_netif_extra_info *) RING_GET_RESPONSE(&vif->rx, @@ -632,8 +660,8 @@ void xenvif_rx_action(struct xenvif *vif) resp->flags |= XEN_NETRXF_extra_info; + gso->u.gso.type = vif->meta[npo.meta_cons].gso_type; gso->u.gso.size = vif->meta[npo.meta_cons].gso_size; - gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4; gso->u.gso.pad = 0; gso->u.gso.features = 0; diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 7e4dcc9..6a005f1 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -577,15 +577,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 |= GSO_BIT(TCPV4); 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 |= GSO_BIT(TCPV4); + + if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6", + "%d", &val) < 0) + val = 0; + if (val) + vif->gso_mask |= GSO_BIT(TCPV6); + + if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv6-prefix", + "%d", &val) < 0) + val = 0; + if (val) + vif->gso_prefix_mask |= GSO_BIT(TCPV6); + + if (vif->gso_mask & vif->gso_prefix_mask) { + xenbus_dev_fatal(dev, err, + "%s: gso and gso prefix flags are not " + "mutually exclusive", + dev->otherend); + return -EOPNOTSUPP; + } /* Before feature-ipv6-csum-offload was introduced, checksum offload * was turned on by a zero value in (or lack of) diff --git a/include/xen/interface/io/netif.h b/include/xen/interface/io/netif.h index d7dd8d7..be341f9 100644 --- a/include/xen/interface/io/netif.h +++ b/include/xen/interface/io/netif.h @@ -113,6 +113,7 @@ struct xen_netif_tx_request { #define XEN_NETIF_EXTRA_FLAG_MORE (1U<<_XEN_NETIF_EXTRA_FLAG_MORE) /* GSO types */ +#define XEN_NETIF_GSO_TYPE_NONE (0) #define XEN_NETIF_GSO_TYPE_TCPV4 (1) #define XEN_NETIF_GSO_TYPE_TCPV6 (2) -- 1.7.10.4
Ian Campbell
2013-Oct-16 16:49 UTC
Re: [PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to the guest
On Fri, 2013-10-11 at 16:06 +0100, Paul Durrant wrote:> This patch adds code to handle SKB_GSO_TCPV6 skbs and construct appropriate > extra or prefix segments to pass the large packet to the frontend. New > xenstore flags, feature-gso-tcpv6 and feature-gso-tcpv6-prefix, are sampled > to determine if the frontend is capable of handling such packets.IIRC the reason we have feature-gso-tcpv4 and feature-gso-tcpv4-prefix is that the former did things in a way which Windows couldn''t cope with. I assuming that is true for v6 too. But could Linux cope with the prefix version too for v6 and reduce the number of options? Or is the non-prefix variant actually better, if the guest can manage, for some reason? I suppose in the end its all piggybacking off the v4 code paths so supporting both isn''t a hardship.> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: David Vrabel <david.vrabel@citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > --- > drivers/net/xen-netback/common.h | 9 +++++-- > drivers/net/xen-netback/interface.c | 6 +++-- > drivers/net/xen-netback/netback.c | 48 +++++++++++++++++++++++++++-------- > drivers/net/xen-netback/xenbus.c | 29 +++++++++++++++++++-- > include/xen/interface/io/netif.h | 1 + > 5 files changed, 77 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h > index b4a9a3c..55b8dec 100644 > --- a/drivers/net/xen-netback/common.h > +++ b/drivers/net/xen-netback/common.h > @@ -87,9 +87,13 @@ struct pending_tx_info { > struct xenvif_rx_meta { > int id; > int size; > + int gso_type; > int gso_size; > }; > > +#define GSO_BIT(type) \ > + (1 << XEN_NETIF_GSO_TYPE_ ## type) > + > /* Discriminate from any valid pending_idx value. */ > #define INVALID_PENDING_IDX 0xFFFF > > @@ -150,9 +154,10 @@ struct xenvif { > u8 fe_dev_addr[6]; > > /* Frontend feature information. */ > + int gso_mask; > + int gso_prefix_mask;Are these storing NETIF_F_FOO? In which case they should be netif_feature_t I think. At the least it needs to be unsigned.> + > u8 can_sg:1; > - u8 gso:1; > - u8 gso_prefix:1; > u8 ip_csum:1; > u8 ipv6_csum:1; > > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index cb0d8ea..e4aa267 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -214,8 +214,10 @@ static netdev_features_t xenvif_fix_features(struct net_device *dev, > > if (!vif->can_sg) > features &= ~NETIF_F_SG; > - if (!vif->gso && !vif->gso_prefix) > + if (~(vif->gso_mask | vif->gso_prefix_mask) & GSO_BIT(TCPV4))I must be blind -- where does this GSO_BIT macro come from? I can''t see it in current net-next...> @@ -392,7 +394,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)I think you can use skb_is_gso(skb) and skb_is_gso_v6(skb) for these ifs.> + gso_type = XEN_NETIF_GSO_TYPE_TCPV6; > + else > + gso_type = XEN_NETIF_GSO_TYPE_NONE; > + > + if (*head && ((1 << gso_type) & vif->gso_mask))Perhaps test_bit(gso_type, &vif->gso_mask) rather than opencoding bitops? I see there are a lot of these, a vif_can_gso_type(vif, gso_type) helper might be nice.> vif->rx.req_cons++; > > *head = 0; /* There must be something in this buffer now. */ > @@ -423,14 +432,28 @@ static int xenvif_gop_skb(struct sk_buff *skb, > unsigned char *data; > int head = 1; > int old_meta_prod; > + int gso_type; > + int gso_size; > > old_meta_prod = npo->meta_prod; > > + 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 = XEN_NETIF_GSO_TYPE_NONE; > + gso_size = 0; > + } > + > /* Set up a GSO prefix descriptor, if necessary */ > - if (skb_shinfo(skb)->gso_size && vif->gso_prefix) { > + if ((1 << skb_shinfo(skb)->gso_type) & vif->gso_prefix_mask) {Isn''t skb->gso_type a Linux GSO flag thing and vif->gso_prefix_mask a Xen netif.h GSO type? Are they really comparable in this way?> req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++); > meta = npo->meta + npo->meta_prod++; > - meta->gso_size = skb_shinfo(skb)->gso_size; > + meta->gso_type = gso_type; > + meta->gso_size = gso_size; > meta->size = 0; > meta->id = req->id; > }> + if (vif->gso_mask & vif->gso_prefix_mask) { > + xenbus_dev_fatal(dev, err, > + "%s: gso and gso prefix flags are not " > + "mutually exclusive",Aren''t they? I thought they were? Maybe I''m reading this error message backwards, in which case I would contend that it is written backwards ;-) Ian.
Paul Durrant
2013-Oct-16 16:52 UTC
Re: [PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to the guest
> -----Original Message----- > From: Ian Campbell > Sent: 16 October 2013 17:49 > To: Paul Durrant > Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel > Subject: Re: [PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to > the guest > > On Fri, 2013-10-11 at 16:06 +0100, Paul Durrant wrote: > > This patch adds code to handle SKB_GSO_TCPV6 skbs and construct > appropriate > > extra or prefix segments to pass the large packet to the frontend. New > > xenstore flags, feature-gso-tcpv6 and feature-gso-tcpv6-prefix, are > sampled > > to determine if the frontend is capable of handling such packets. > > IIRC the reason we have feature-gso-tcpv4 and feature-gso-tcpv4-prefix > is that the former did things in a way which Windows couldn''t cope with. > I assuming that is true for v6 too. But could Linux cope with the prefix > version too for v6 and reduce the number of options? Or is the > non-prefix variant actually better, if the guest can manage, for some > reason? >The non-prefix variant actually conveys type information and so will be better for frontends that don''t have to re-parse the headers anyway. Paul> I suppose in the end its all piggybacking off the v4 code paths so > supporting both isn''t a hardship. >
Ian Campbell
2013-Oct-16 17:01 UTC
Re: [PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to the guest
On Wed, 2013-10-16 at 17:52 +0100, Paul Durrant wrote:> > -----Original Message----- > > From: Ian Campbell > > Sent: 16 October 2013 17:49 > > To: Paul Durrant > > Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel > > Subject: Re: [PATCH net-next v4 5/5] xen-netback: enable IPv6 TCP GSO to > > the guest > > > > On Fri, 2013-10-11 at 16:06 +0100, Paul Durrant wrote: > > > This patch adds code to handle SKB_GSO_TCPV6 skbs and construct > > appropriate > > > extra or prefix segments to pass the large packet to the frontend. New > > > xenstore flags, feature-gso-tcpv6 and feature-gso-tcpv6-prefix, are > > sampled > > > to determine if the frontend is capable of handling such packets. > > > > IIRC the reason we have feature-gso-tcpv4 and feature-gso-tcpv4-prefix > > is that the former did things in a way which Windows couldn''t cope with. > > I assuming that is true for v6 too. But could Linux cope with the prefix > > version too for v6 and reduce the number of options? Or is the > > non-prefix variant actually better, if the guest can manage, for some > > reason? > > > > The non-prefix variant actually conveys type information and so will > be better for frontends that don''t have to re-parse the headers > anyway.Make sense, thanks.> > Paul > > > I suppose in the end its all piggybacking off the v4 code paths so > > supporting both isn''t a hardship. > > >