Re: [Xen-devel] [PATCH v4 2/3] public/io/netif.h: document control ring and toeplitz hashing

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2016-01-08 at 16:19 +0000, Paul Durrant wrote:
> > > + * The control ring uses a fixed request/response message size and
> > > is
> > > + * balanced (i.e. one request to one response), so operationally it
> > > is much
> > > + * the same as a tramsmit or receive ring.
> > 
> > "transmit".
> > 
> > Is it intended to support out of order responses?
> 
> I didn't want to preclude it in case someone wants to add a potentially
> long running control operation in future.

Probably best to explicitly flag this possibility.


> 
> > 
> > > + */
> > > +
> > > +/*
> > > + * Toeplitz hash types
> > > + * ===================
> > > + *
> > > + * For the purposes of the definitions below, 'Packet[]' is an array
> > > of
> > > + * octets containing an IP packet without options, 'Array[X..Y]'
> > > means a
> > > + * sub-array of 'Array' containing bytes X thru Y inclusive, and '+'
> > > is
> > > + * used to indicate concatenation of arrays.
> > > + */
> > > +
> > > +/*
> > > + * A hash calculated over an IP version 4 header as follows:
> > > + *
> > > + * Buffer[0..8] = Packet[12..15] + Packet[16..19]
> > 
> > Is there a reason to write it like this rather than:
> > + * Buffer[0..8] = Packet[12..19]
> > ?
> > 
> > Maybe something which is obvious if you know about Toeplitz in more
> > than
> > just a passing fashion?
> 
> The reason is actually so it more resembles the Microsoft documentation
> at https://msdn.microsoft.com/en-
> us/library/windows/hardware/ff570725%28v=vs.85%29.aspx.

Ah, that's not unreasonable.

>  Perhaps it would make more sense if I pointed out that Packet[12..15] is
> the source address and Packet[16..19] is the dest address (and do the
> same in the subsequent comments)?

Assuming it doesn't negatively impact the readability, sure.

> > I know the Toeplitz stuff doesn't need it, but we should consider
> > designing
> > in a scheme to handle control commands which need >8 octets of data, or
> > else we risk ending up with something as confusing as the data path...
> > 
> 
> I'm ok with that, but you can always use a gref (as is done for the key
> and mapping table). If 8 seems to small, what do you feel would be the
> right number to choose?

I'm happy with indirect via a gref as a scheme if that seems sufficient.

+ *
> > > + * type = NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS:
> > > + *
> > > + * This is sent by the frontend to set the types of toeplitz hash
> > > that
> > > + * the backend should calculate. 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 former only calculated for non-TCP
> > > packets.
> > > + * The data[0] field is a bitwise OR of NETIF_CTRL_TOEPLITZ_FLAG_*
> > values
> > > + * defined above. The data[1] field is set to 0.
> > > + *
> > > + * NOTE: Setting data[0] to 0 disables toeplitz hashing and the
> > > backend
> > > + *ÂÂÂÂÂÂÂis free to choose how it steers packets to queues (which is
> > > the
> > > + *ÂÂÂÂÂÂÂdefault state).
> > > + *
> > > + * type = NETIF_CTRL_TYPE_SET_`TOEPLITZ_KEY:
> > 
> > Can this be sent before FLAGS?
> > 
> > Is it permissible to send this for a hash scheme not included in flags?
> > 
> 
> Yes, it can but the idea is that nothing has any effect until a valid set
> of flags is specified.

So the b/e is required to remember keys corresponding to disabled hashes
and immediately use them when the hash is enabled, it is not allowed to
ignore this message and expect another key once the hash is enabled.

If you enable a hash without setting a key then what does the key default
to? Traffic could be flowing while this is going on, right (since things
are Connected)?

> > > +/*
> > > + * Control responses (netif_ctrl_response_t)
> > > + * =========================================
> > > + *
> > > + * All responses have the following format:
> > > + *
> > > + *ÂÂÂÂ0ÂÂÂÂÂ1ÂÂÂÂÂ2ÂÂÂÂÂ3ÂÂÂÂÂ4ÂÂÂÂÂ5ÂÂÂÂÂ6ÂÂÂÂÂ7ÂÂoctet
> > > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > > + * |ÂÂÂÂidÂÂÂÂÂ|ÂÂÂpadÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂstatusÂÂÂÂÂÂÂÂ|
> > > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > > + * |ÂÂÂÂÂÂÂÂÂdataÂÂÂÂÂÂÂÂÂÂ|
> > > + * +-----+-----+-----+-----+
> > > + *
> > > + * id: the corresponding request identifier
> > > + * pad: set to 0
> > > + * status: the status of request processing
> > > + * data: any data associated with the response (determined by type
> > > and
> > > + *ÂÂÂÂÂÂÂstatus)
> > 
> > type needs to be remembered via the id? Why not echo it in the pad
> > field
> > for convenience?
> > 
> 
> I originally had that but thought it would be superfluous given that the
> id is echoed. I can add it back if you think that it would be a good
> idea. All my implementation did with it though was ASSERT that it matched
> the req.

Was it a coincidence that you had the req in hand given the id, or is it a
fundamental property of any implementation of this protocol that you would
have it handy?
>Â
> > > + *
> > > + * type = NETIF_CTRL_MSG_SET_TOEPLITZ_FLAGS:
> > > + *
> > > + * If the data[0] field in the request is invalid (i.e. contains
> > > unsupported
> > > + * hash types) then the status field is set to
> > > + * NETIF_CTRL_STATUS_INVALID_PARAMETER. Otherwise the requset
> > should
> > 
> > "request"
> > 
> > > succeed
> > > + * and hence the status field is set to NETIF_CTRL_STATUS_SUCCESS.
> > > + * The data field should be set to 0.
> > > + *
> > > + * type = NETIF_CTRL_MSG_SET_TOEPLITZ_KEY:
> > > + *
> > > + * If the data[0] field in the request is an invalid key length (too
> > > big)
> > 
> > Can it not be too small as well? What is the behaviour if it is?
> > 
> 
> I guess the way the algorithm is defined, a key of less than 32 bits
> doesn't make much sense.

What if I throw caution to the wind and set one byte anyway?

> 
> > > + * then the status field is set to
> > NETIF_CTRL_STATUS_BUFFER_OVERFLOW, If the
> > > + * data[1] field is an invalid grant reference then the status field
> > > is set
> > > + * to NETIF_CTRL_STATUS_INVALID_PARAMETER. Otherwise the request
> > should
> > > + * succeed and hence the status field is set to
> > NETIF_CTRL_STATUS_SUCCESS.
> > > + * The data field should be set to 0.
> > > + *
> > > + * type = NETIF_CTRL_MSG_SET_TOEPLITZ_MAPPING:
> > > + *
> > > + * If the data[0] field in the request is an invalid mapping order
> > > (too big)
> > 
> > or too small?
> 
> An order 0 mapping would still contain a single entry and you can't get
> any smaller than that :-)

I missed the order, assumed it was size, but still, what if there are 16
queues and I pass order 0, the behaviour of queues 2..16 needs to be well
defined.

Since this is a single page, what happens with >=512 queues? (and how far
away is that possibility).

Ian.


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