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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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?

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.

> > @@ -341,11 +438,14 @@ typedef struct netif_ctrl_response
> netif_ctrl_response_t;
> >   *           NETIF_CTRL_STATUS_SUCCESS       - Operation successful
> >   *  data   = supported hash types (if operation was successful)
> 
> 
> 
> >   *
> > - * NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS
> > - * ----------------------------------
> > + * NOTE: A valid hash algorithm must be selected before this operation
> can
> > + *       succeed.
> >   *
> > - * This is sent by the frontend to set the types of toeplitz hash that
> > - * the backend should calculate. (See above for hash type definitions).
> > + * NETIF_CTRL_TYPE_SET_HASH_FLAGS
> > + * ------------------------------
> > + *
> > + * This is sent by the frontend to set the types of hash that the backend
> > + * should calculate. (See above for hash type definitions).
> >   * Note that the 'maximal' type of hash should always be chosen. For
> >   * example, if the frontend sets both IPV4 and IPV4_TCP hash types then
> >   * the latter hash type should be calculated for any TCP packet and the
> > @@ -353,8 +453,8 @@ typedef struct netif_ctrl_response
> netif_ctrl_response_t;
> >   *
> >   * Request:
> >   *
> > - *  type    = NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS
> > - *  data[0] = bitwise OR of NETIF_CTRL_TOEPLITZ_HASH_* values
> > + *  type    = NETIF_CTRL_TYPE_SET_HASH_FLAGS
> > + *  data[0] = bitwise OR of NETIF_CTRL_HASH_TYPE_* values
> 
> Did you mean s/TYPE/ALGORITHM/?
> 

No. This is for flags as it was before.

> Currently defined is none (0) and toeplitz (1) so it isn't clear if the
> next two would be 2 then 4 or 2 then 3 (i.e. if those are bit offsets
> or values) and it hasn't been clear in each context so far which is
> needed.
> 
> Using _NETIF_CTRL_HASH_ALGORITHM as a bit offset and using that to
> define NETIF_CTRL_HASH_ALGORITHM and referencing the _ or not-_
> versions might help?
> 
> > + * NOTE: A valid hash algorithm must be selected before this operation
> can
> > + *       succeed.
> > + *       Also, setting data[0] to zero disables hashing and the backend
> > + *       is free to choose how it steers packets to queues.
> >   *
> > - * (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').
> > + * NETIF_CTRL_TYPE_SET_HASH_KEY
> > + * ----------------------------
> >   *
> > - * 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
> > + * This is sent by the frontend to set the key of the hash if the
> > algorithm
> > + * requires it. (See hash algorithms above).
> >   *
> >   * Request:
> >   *
> > - *  type    = NETIF_CTRL_TYPE_SET_TOEPLITZ_KEY
> > + *  type    = NETIF_CTRL_TYPE_SET_HASH_KEY
> >   *  data[0] = grant reference of page containing the key (assumed to
> >   *            start at beginning of grant)
> >   *  data[1] = size of key in octets
> > @@ -411,13 +500,13 @@ typedef struct netif_ctrl_response
> > netif_ctrl_response_t;
> >   *       invalidates any previous key, hence specifying a key size
> > of
> >   *       zero will clear the key (which ensures that the calculated
> > hash
> >   *       will always be zero).
> > - *       The maximum size of key is backend specific, but is also
> > limited
> > - *       by the single grant reference.
> > + *       The maximum size of key is algorithm and backend specific,
> > but
> > + *       is also limited by the single grant reference.
> >   *       The grant reference may be read-only and must remain valid
> > until
> >   *       the response has been processed.
> >   *
> > - * NETIF_CTRL_TYPE_GET_TOEPLITZ_MAPPING_ORDER
> > - * ------------------------------------------
> > + * NETIF_CTRL_TYPE_GET_HASH_MAPPING_ORDER
> > + * --------------------------------------
> >   *
> >   * This is sent by the frontend to query the maximum order of
> > mapping
> >   * table supported by the backend. The order is specified in terms
> > of
> > @@ -425,7 +514,7 @@ typedef struct netif_ctrl_response
> > netif_ctrl_response_t;
> >   *
> >   * Request:
> >   *
> > - *  type    = NETIF_CTRL_TYPE_GET_TOEPLITZ_MAPPING_ORDER
> > + *  type    = NETIF_CTRL_TYPE_GET_HASH_MAPPING_ORDER
> >   *  data[0] = 0
> >   *  data[1] = 0
> >   *  data[2] = 0
> > @@ -436,8 +525,8 @@ typedef struct netif_ctrl_response
> > netif_ctrl_response_t;
> >   *           NETIF_CTRL_STATUS_SUCCESS       - Operation successful
> >   *  data   = maximum order of mapping table (if operation was
> > successful)
> >   *
> > - * 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?

> 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