Re: [PATCH v2] public/io/netif.h: make control ring hash protocol more general

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2016-02-16 at 11:02 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: 16 February 2016 10:23
> > To: Paul Durrant; xen-devel@lists.xenproject.org
> > Cc: Ian Jackson; Jan Beulich; Keir (Xen.org); Tim (Xen.org)
> > Subject: Re: [PATCH v2] public/io/netif.h: make control ring hash
> > protocol
> > more general
> > 
> > On Mon, 2016-02-15 at 11:14 +0000, Paul Durrant wrote:
> > > -#define _NETIF_CTRL_TOEPLITZ_HASH_IPV6     2
> > > -#define NETIF_CTRL_TOEPLITZ_HASH_IPV6      (1 <<
> > _NETIF_CTRL_TOEPLITZ_HASH_IPV4)
> > > +#define _NETIF_CTRL_HASH_TYPE_IPV6     2
> > > +#define NETIF_CTRL_HASH_TYPE_IPV6 \
> > > +        (1 << _NETIF_CTRL_HASH_TYPE_IPV4)
> > 
> > I think the unwrapped line was 80 characters in total. FWIW I'd prefer
> > just pulling in the indentation four spaces (or reducing to just one)
> > over the wrapper.
> 
> Ok.
> 
> > > 
> > > -#define _NETIF_CTRL_TOEPLITZ_HASH_IPV6_TCP 3
> > > -#define NETIF_CTRL_TOEPLITZ_HASH_IPV6_TCP  (1 <<
> > > _NETIF_CTRL_TOEPLITZ_HASH_IPV4_TCP)
> > > +
> > > +#define NETIF_CTRL_HASH_ALGORITHM_TOEPLITZ 1
> > > +
> > > +/*
> > > + * This algorithm uses a 'key' as well as the data buffer itself.
> > > + * (Buffer[] and Key[] are treated as shift-registers where the MSB
> > > of
> > > + * Buffer/Key[0] is considered 'left-most' and the LSB of
> > > Buffer/Key[N-1]
> > > + * is the 'right-most').
> > > + *
> > > + * Value = 0
> > > + * For number of bits in Buffer[]
> > > + *    If (left-most bit of Buffer[] is 1)
> > > + *        Value ^= left-most 32 bits of Key[]
> > > + *    Key[] << 1
> > > + *    Buffer[] << 1
> > > + *
> > > + * The code below is provided for convenience where an operating
> > system
> > > + * does not already provide an implementation.
> > 
> > Is this really useful in practice? It just seems odd to have so much
> > implementation in an interface header and I would have thought this was
> > well defined enough that anyone could create a suitable implementation
> > in their OS
> > 
> 
> I think it's useful to have the algorithm in actual code as well as
> pseudo (since it's actually a little bit of a PITA to implement on little
> endian h/w anyway).
> 
> > > + */
> > > +#ifdef NETIF_DEFINE_TOEPLITZ
> > 
> > If we go with this then this should have an addtional XEN_ on the
> > front.
> 
> The header is inconsistent at the moment. Some things are prefixed with
> XEN_ some are not so if you want this prefixed then I think it's best I
> add another patch before this to change all unqualified netif/NETIF
> occurrences to xen_netif/XEN_NETIF... it will also mean less post-
> processing when I re-import the header into Linux.
> 
> > 
> > > +static uint32_t netif_toeplitz_hash(const uint8_t *key,
> > > +                                    unsigned int keylen,
> > > +                                    const uint8_t *buf,
> > > +                                    unsigned int buflen)
> > > 
> > [...]
> > 
> > > + *
> > > + * NOTE: Setting data[0] to NETIF_CTRL_HASH_ALGORITHM_INVALID
> > disables
> > 
> > I think it was called _NONE not _INVALID?
> 
> Yes indeed. That needs fixing.
> 
> > 
> > > + *       hashing and the backend is free to choose how it steers
> > > packets to
> > > + *       queues (which is the default behaviour).
> > > + *
> > > + * NETIF_CTRL_TYPE_GET_HASH_FLAGS
> > > + * ------------------------------
> > > + *
> > > + * This is sent by the frontend to query the types of hash supported
> > > by
> > > + * the backend.
> > > + *
> > > + * Request:
> > > + *
> > > + *  type    = NETIF_CTRL_TYPE_GET_HASH_FLAGS
> > >   *  data[0] = 0
> > >   *  data[1] = 0
> > >   *  data[2] = 0
> > 
> > I may be misreading how this patch applies to the existing text, but
> > I'm not seeing how the set of supported hashes is encoded in the
> > response. I suppose it is by setting to corresponding bit
> > (1<<NETIF_CTRL_HASH_ALGORITHM_*)? I think there is scope for some
> > endianness style confusion with data[0] vs data[2] etc in that though
> > so could do with being made more explicit somehow.
> > 
> 
> No, this has not changed. The flags are reported just the way they were
> before (IPv4|IPv4+TCP|IPv6|IPv6+TCP). Were you assuming the set of
> supported algorithms was reported using this?

Yes. I'm not sure why since it is pretty clear from the name used above!

> I didn't add a message for getting back supported algorithms as I
> envisaged a frontend just attempting to set the one it wants to use and,
> if it gets back 'invalid' from the backend, then it would just give up
> and not configure hashing.

Makes sense.

> 
> > >   *
> > > - * NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING_ORDER
> > > - * ------------------------------------------
> > > + * NETIF_CTRL_TYPE_SET_HASH_MAPPING_ORDER
> > 
> > This one needs a similar "if the hash algorithm requires it" wording
> > like the setting the key one had.
> > 
> 
> Why? Is there any point of doing hashing at all if the backend is not
> going to map it to a queue via a mapping table?

But will all hashing algorithms work via a table with a variable order?

> 
> > Listing the valid key/order/etc operations for each hash type up next
> > to the hash definition might help clarify things even further?
> 
> The description of Toeplitz already details how the key is used and
> everything else is generic. Do I need more?
> 
>   Paul

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