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

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

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.
>  
> -#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

> + */
> +#ifdef NETIF_DEFINE_TOEPLITZ

If we go with this then this should have an addtional XEN_ on the
front.

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

> + *       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.

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

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.

Listing the valid key/order/etc operations for each hash type up next
to the hash definition might help clarify things even further?

Ian.