Re: [Xen-devel] [PATCH v5 3/5] public/io/netif.h: add documentation for hash negotiation and mapping

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 22 October 2015 09:48
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir
> (Xen.org); Tim (Xen.org)
> Subject: Re: [PATCH v5 3/5] public/io/netif.h: add documentation for hash
> negotiation and mapping
> 
> >>> On 20.10.15 at 14:35, <paul.durrant@xxxxxxxxxx> wrote:
> > +/*
> > + * Hash negotiation (only applicable if using multiple queues):
> > + *
> > + * A backend can advertise a set of hash algorithms that it can perform by
> > + * naming it in a space separated list in the "multi-queue-hash-list"
> > + * xenstore key. For example, if the backend supports the 'foo' and 'bar'
> > + * algorithms it would set:
> > + *
> > + * /local/domain/X/backend/vif/Y/Z/multi-queue-hash-list = "foo bar"
> 
> Wouldn't comma separated be the more usual form?
> 

I was referring back to the original proposal made by Andrew Bennieston a 
couple of years ago (see 
http://lists.xen.org/archives/html/xen-devel/2013-06/msg02654.html) which 
proposed a space separated list and I don't think anyone disagreed). I'm happy 
with comma separated if everyone else is though.

> > + * Additionally, in supporting a particular algorithm, it may be necessary
> > + * for the backend to specify the capabilities of its implementation of
> > + * that algorithm, e.g. what sections of packet header it can hash.
> > + * To do that it can set algorithm-specific keys under a parent 
> > capabilities
> > + * key. For example, if the 'bar' algorithm implementation in the backend
> > + * is capable of hashing over an IP version 4 header and a TCP header, the
> > + * backend might set:
> > + *
> > + * /local/domain/X/backend/vif/Y/Z/multi-queue-hash-caps-bar/types =
> "ipv4+tcp"
> > + *
> > + * The backend should set all such keys before it moves into the initwait
> > + * state.
> > + *
> > + * The frontend can select a hash algorithm at any time after it moves into
> > + * the connected state by setting the "multi-queue-hash" key. The
> backend
> > + * must therefore watch this key and be prepared to change hash
> algorithms
> > + * at any time whilst in the connected state. So, for example, if the
> > + * frontend wants 'foo' hashing, it should set:
> > + *
> > + * /local/domain/Y/device/vif/Z/multi-queue-hash = "foo"
> > + *
> > + * Additionally it may set parameters for that algorithm by setting
> > + * algorithm-specific keys under a parent parameters key. For example, if
> > + * the 'foo' algorithm implementation in the backend is capable of hashing
> > + * over an IP version 4 header, a TCP header or both but the frontend only
> > + * wants it to hash over only the IP version 4 header then it might set:
> > + *
> > + * /local/domain/Y/device/vif/Z/multi-queue-hash-params-foo/types =
> "ipv4"
> > + *
> > + * The backend must also watch the parameters key as the frontend may
> > + * change the parameters at any time whilst in the connected state.
> > + *
> > + * (Capabilities and parameters documentation for specific algorithms is
> > + * below).
> > + *
> > + * TOEPLITZ:
> > + *
> > + * If the backend supports Toeplitz hashing then it should include
> > + * the algorithm name 'toeplitz' in its "multi-queue-hash-list" key.
> > + * It should also advertise the following capabilities:
> > + *
> > + * types: a space separated list containing any or all of 'ipv4', 'tcpv4',
> > + *        'ipv6', 'tcpv6', indicating over which headers the hash algorithm
> > + *        is capable of being performed.
> 
> Same question regarding space uses as separator. Also I think the
> separator(s) permitted should be mentioned above, where the types
> key gets introduced.

Ok.

> 
> > + * max-key-length: an integer value indicating the maximum key length (in
> > + *                 octets) that the frontend may supply.
> > + *
> > + * Upon selecting this algorithm, the frontend may supply the following
> > + * parameters.
> > + *
> > + * types: a space separated list containing none, any or all of the type
> > + *        names included in the types list in the capabilities.
> > + *        When the backend encounters a packet type not in this list it
> > + *        will assign a hash value of 0.
> > + *
> > + * key: a ':' separated list of octets (up to the maximum length specified
> > + *      in the capabilities) expressed in hexadecimal indicating the key
> > + *      that should be used in the hash calculation.
> 
> While I see no way around this proliferation of keys, have you
> considered the resource consumption effect? Guests have a limit on
> how much space they may consume in xenstore, and with additions
> like these it seems increasingly likely for the defaults to no longer be
> sufficient.
> 

I have considered it and I think it will probably mean adjustments when we pull 
this into XenServer. Do you think it's worth making a change in the default 
oxenstored.conf as part of this series?

> > +/*
> > + * Hash mapping (only applicable if using multiple queues):
> > + *
> > + * If the backend is not capable, or no mapping is specified by the
> frontend
> > + * then it is assumed that the hash -> queue mapping is done by simple
> > + * modular arithmetic.
> > + *
> > + * To advertise that it is capable of accepting a specific mapping from the
> > + * frontend the backend should set the "multi-queue-max-hash-mapping-
> length"
> > + * key to a non-zero value. The frontend may then specify a mapping (up
> to
> > + * the maximum specified length) as a ',' separated list of decimal queue
> > + * numbers in the "multi-queue-hash-mapping" key.
> 
> Again due to space constraints, is decimal really a good choice here?
> Hex would be more dense, and one might consider an even higher
> radix.
> 

I'm ok with hex. Using an alternative higher radix may cause confusion I 
suspect.

  Paul

> Jan

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