Re: [Xen-devel] [PATCH net-next v4 1/5] xen-netback: add support for IPv6 checksum offload to guest

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: 14 October 2013 11:54
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; David Vrabel
> Subject: Re: [PATCH net-next v4 1/5] xen-netback: add support for IPv6
> checksum offload to guest
> 
> On Fri, 2013-10-11 at 16:06 +0100, Paul Durrant wrote:
> > Check xenstore flag feature-ipv6-csum-offload to determine if a
> > guest is happy to accept IPv6 packets with only partial checksum.
> > Also check analogous feature-ip-csum-offload to determine if a
> > guest is happy to accept IPv4 packets with only partial checksum
> > as a replacement for a negated feature-no-csum-offload value and
> > add a comment to deprecate use of feature-no-csum-offload.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> Shouldn't this come later in the series, i.e. after netback is actually
> able to cope with ipv6 offloads?
> 

I guess that's debatable. The patches don't have any dependency relation; 
offloads to and from the guest are quite independent.

> > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
> netback/common.h
> > index 5715318..b4a9a3c 100644
> > --- a/drivers/net/xen-netback/common.h
> > +++ b/drivers/net/xen-netback/common.h
> > @@ -153,7 +153,8 @@ struct xenvif {
> >     u8 can_sg:1;
> >     u8 gso:1;
> >     u8 gso_prefix:1;
> > -   u8 csum:1;
> > +   u8 ip_csum:1;
> > +   u8 ipv6_csum:1;
> 
> Why not ipv4_csum for consistency/unambiguity?
> 

I followed general linux naming conventions e.g. ip_hdr and ipv6_hdr.

> > diff --git a/include/xen/interface/io/netif.h
> b/include/xen/interface/io/netif.h
> > index eb262e3..d9fb44739 100644
> > --- a/include/xen/interface/io/netif.h
> > +++ b/include/xen/interface/io/netif.h
> > @@ -51,6 +51,16 @@
> >   */
> >
> >  /*
> > + * "feature-no-csum-offload" was used to turn off IPv4 TCP/UDP
> checksum
> > + * offload but is now deprecated. Two new feature flags should now be
> used
> > + * to control checksum offload:
> 
> How is a frontend to know which sort of backend it is talking too? Is
> there going to be a feature flag to indicate support for these new
> flags?
> 
> In particular a new frontend running on an old backend needs to know
> that it needs to set no-csum-offload instead of ip-csum-offload somehow.
> 

Good point. Without any version I guess we have to live with the old flag 
forever. I'll stick with it for v4 and just leave the new one for v6.

  Paul

> > + * "feature-ip-csum-offload" should be used to turn IPv4 TCP/UDP
> checksum
> 
> "ipv4" again?
> 
> > + * offload on or off. If it is missing then the feature is assumed to be 
> > on.
> > + * "feature-ipv6-csum-offload" should be used to turn IPv6 TCP/UDP
> checksum
> > + * offload on or off. If it is missing then the feature is assumed to be 
> > off.
> > + */
> > +
> > +/*
> >   * This is the 'wire' format for packets:
> >   *  Request 1: xen_netif_tx_request  -- XEN_NETTXF_* (any flags)
> >   * [Request 2: xen_netif_extra_info]    (only if request 1 has
> XEN_NETTXF_extra_info)
> 

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