| Message ID | 1452171912-29857-3-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-01-07 at 13:05 +0000, Paul Durrant wrote: > > diff --git a/xen/include/public/io/netif.h > b/xen/include/public/io/netif.h > index 1790ea0..06e0b61 100644 > --- a/xen/include/public/io/netif.h > +++ b/xen/include/public/io/netif.h > @@ -151,6 +151,270 @@ > */ > > /* > + * Control ring > + * ============ > + * > + * Some features, such as toeplitz hashing (detailed below), require a > + * significant amount of out-of-band data to be passed from frontend to > + * backend. Use of xenstore is not suitable for large quantities of data > + * because of quota limitations and so a dedicated 'control ring' is used. > + * The ability of the backend to use a control ring is advertised by > + * setting: > + * > + * /local/domain/X/backend///feature-control-ring = "1" > + * > + * The frontend provides a control ring to the backend by setting: > + * > + * /local/domain//device/vif//ctrl-ring-ref = > + * /local/domain//device/vif//event-channel-ctrl = > + * > + * where is the grant reference of the shared page used to > + * implement the control ring and is an event channel to be used > + * as a mailbox interrupt, before the frontend moves into the connected > + * state. I read this as saying that the mailbox interrupt can be used until the frontend moves into the connected state, which I realise isn't what you meant but having so much stuff between "by setting" and "before" makes it easy to read wrongly. How about "... as a mailbox interrupt. These keys must be written before moving into the connected state." ? > + * 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? > + */ > + > +/* > + * 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? > +/* > + * Control requests (netif_ctrl_request_t) > + * ======================================= > + * > + * All requests have the following format: > + * > + * 0 1 2 3 4 5 6 7 octet > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | id | type | data[0] | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | data[1] | > + * +-----+-----+-----+-----+ These are packed in the ring, i.e. there are no implicit padding bytes, right? 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... > + * > + * id: the request identifier, echoed in response. > + * type: the type of request (see below) > + * data[]: any data associated with the request (determined by type) > + */ > + > +struct netif_ctrl_request { > + uint16_t id; > + uint16_t type; > + > +#define NETIF_CTRL_TYPE_INVALID 0 > +#define NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS 1 > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS 2 > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_KEY 3 > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING 4 > + > + uint32_t data[2]; > +}; > +typedef struct netif_ctrl_request netif_ctrl_request_t; > + > +/* > + * type = NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS: > + * > + * This is sent by the frontend to query the types of toeplitz > + * hash supported by the backend. No data is required and to the "and to the" has an extra "to" in it I think? > + * data[] field is set to 0. "fields are" ? > + * > + * 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? > + * > + * This is sent by the frontend to set the key of toeplitz hash that > + * the backend should calculate. The toeplitz algorithm is illustrated > + * by the following pseudo-code: > + * > + * (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 data[0] field is set to the size of key in octets. The data[1] > + * field is set to a grant reference of a page containing the key. I suppose it is implicit that the key starts at offset 0 in the page? > The > + * reference must remain valid until the corresponding > + * netif_ctrl_response_t has been processed. May the ref be r/o? If so, should that be encouraged? > + * > + * type = NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING: > + * > + * This is sent by the frontend to set the mapping of toeplitz hash to > + * queue number to be applied by the backend. > + * > + * The data[0] field is set to the order of the mapping. The data[1] field > + * is set to a grant reference of a page containing the mapping. The > + * reference must remain valid until the corresponding > + * netif_ctrl_response_t has been processed. > + * > + * The format of the mapping is: > + * > + * 0 1 2 3 4 5 6 7 octet > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | queue[0] | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | queue[1] | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | queue[2] | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | queue[3] | > + * . > + * . > + * | queue[N-1] | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * > + * where each queue value is less than "multi-queue-num-queues" (see above) > + * and N is 1 << data[0]. > + * > + * NOTE: Before a specific mapping is set using this request, the backend > + * should map all toeplitz hash values to queue 0 (which is the only > + * queue guaranteed to exist in all cases). i.e. between receiving a NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS with a non-zero set of flags and NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING it should do this? If it has not seen a NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS, or it has seen one with flags = 0 then as before the b/e is free to steer as it likes? Is it valid to send this mapping command first before the flags? > + */ > + > +/* > + * 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? > + */ > + > +struct netif_ctrl_response { > + uint16_t id; > + uint16_t pad; > + uint32_t status; > + > +#define NETIF_CTRL_STATUS_SUCCESS 0 > +#define NETIF_CTRL_STATUS_NOT_SUPPORTED 1 > +#define NETIF_CTRL_STATUS_INVALID_PARAMETER 2 > +#define NETIF_CTRL_STATUS_BUFFER_OVERFLOW 3 Is the intention that these will be the union of all possible failure modes, rather than making provision for type-specific failures? These are all pretty generic though, and I suppose we could always figure that out when some command has a very obscure failure case. > + > + uint32_t data; > +}; > +typedef struct netif_ctrl_response netif_ctrl_response_t; > + > +/* > + * type = > + * > + * The default response for any unrecognised request has the status field > + * set to NETIF_CTRL_STATUS_NOT_SUPPORTED and the data field set to 0. > + * > + * type = NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS: > + * > + * Since the request carries no data there is no reason for processing to > + * fail, hence the status field is set to NETIF_CTRL_STATUS_SUCCESS and the > + * data field is a bitwise OR of NETIF_CTRL_TOEPLITZ_FLAG_* values (defined > + * above) indicating which hash types are supported by the backend. > + * If no hashing is supported then the data field should be set to 0. I wonder if it would be clearer to define the generic request and response structures up front followed by the specific commands and their request+response formats at the same time, rather than splitting the specific command requests and responses into different places? Like: Command: NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS Description: Blah blah Request data format: Words words Valid response status codes: - One - Another Responds data format: Words (i.e. more like API than protocol documentation in the form of independent looking things going back and forth) > + * > + * 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? > + * 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? > + * 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 requset should "request" Ian.
On 07/01/16 13:05, Paul Durrant wrote: > This patch documents a new shared ring between frontend and backend that > can be used to pass bulk out-of-band data, such as that required to > implement toeplitz hashing in the backend such that it is configurable by > the frontend. > > The patch then goes on to document the messages passed over the control > ring that can be used to configure toeplitz hashing. [...] > --- a/xen/include/public/io/netif.h > +++ b/xen/include/public/io/netif.h > @@ -151,6 +151,270 @@ > */ > > /* > + * Control ring > + * ============ [...] > + * The frontend provides a control ring to the backend by setting: > + * > + * /local/domain/<domid>/device/vif/<vif>/ctrl-ring-ref = <gref> > + * /local/domain/<domid>/device/vif/<vif>/event-channel-ctrl = <port> ctrl-event-channel to match ctrl-ring-ref? David
> -----Original Message----- > From: Ian Campbell [mailto:ian.campbell@citrix.com] > Sent: 08 January 2016 15:54 > To: Paul Durrant; xen-devel@lists.xenproject.org > Cc: Ian Jackson; Jan Beulich; Keir (Xen.org); Tim (Xen.org) > Subject: Re: [PATCH v4 2/3] public/io/netif.h: document control ring and > toeplitz hashing > > On Thu, 2016-01-07 at 13:05 +0000, Paul Durrant wrote: > > > > diff --git a/xen/include/public/io/netif.h > > b/xen/include/public/io/netif.h > > index 1790ea0..06e0b61 100644 > > --- a/xen/include/public/io/netif.h > > +++ b/xen/include/public/io/netif.h > > @@ -151,6 +151,270 @@ > > */ > > > > /* > > + * Control ring > > + * ============ > > + * > > + * Some features, such as toeplitz hashing (detailed below), require a > > + * significant amount of out-of-band data to be passed from frontend to > > + * backend. Use of xenstore is not suitable for large quantities of data > > + * because of quota limitations and so a dedicated 'control ring' is used. > > + * The ability of the backend to use a control ring is advertised by > > + * setting: > > + * > > + * /local/domain/X/backend///feature-control-ring = "1" > > + * > > + * The frontend provides a control ring to the backend by setting: > > + * > > + * /local/domain//device/vif//ctrl-ring-ref = > > + * /local/domain//device/vif//event-channel-ctrl = > > + * > > + * where is the grant reference of the shared page used to > > + * implement the control ring and is an event channel to be used > > + * as a mailbox interrupt, before the frontend moves into the connected > > + * state. > > I read this as saying that the mailbox interrupt can be used until the > frontend moves into the connected state, which I realise isn't what you > meant but having so much stuff between "by setting" and "before" makes it > easy to read wrongly. > > How about "... as a mailbox interrupt. These keys must be written before > moving into the connected state." ? Yes, that's clearer. > > > + * 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. > > > + */ > > + > > +/* > > + * 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. 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)? > > > > +/* > > + * Control requests (netif_ctrl_request_t) > > + * ======================================= > > + * > > + * All requests have the following format: > > + * > > + * 0 1 2 3 4 5 6 7 octet > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | id | type | data[0] | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | data[1] | > > + * +-----+-----+-----+-----+ > > These are packed in the ring, i.e. there are no implicit padding bytes, > right? DEFINE_RING_TYPES unions the req and rsp structures and then defines an array but since req and rsp are the same size in this case the reqs should end up packed. > > 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? > > + * > > + * id: the request identifier, echoed in response. > > + * type: the type of request (see below) > > + * data[]: any data associated with the request (determined by type) > > + */ > > + > > +struct netif_ctrl_request { > > + uint16_t id; > > + uint16_t type; > > + > > +#define NETIF_CTRL_TYPE_INVALID 0 > > +#define NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS 1 > > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS 2 > > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_KEY 3 > > +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING 4 > > + > > + uint32_t data[2]; > > +}; > > +typedef struct netif_ctrl_request netif_ctrl_request_t; > > + > > +/* > > + * type = NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS: > > + * > > + * This is sent by the frontend to query the types of toeplitz > > + * hash supported by the backend. No data is required and to the > > "and to the" has an extra "to" in it I think? > > > > + * data[] field is set to 0. > > "fields are" ? > > > + * > > + * 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. > > + * > > + * This is sent by the frontend to set the key of toeplitz hash that > > + * the backend should calculate. The toeplitz algorithm is illustrated > > + * by the following pseudo-code: > > + * > > + * (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 data[0] field is set to the size of key in octets. The data[1] > > + * field is set to a grant reference of a page containing the key. > > I suppose it is implicit that the key starts at offset 0 in the page? > Yes. I'll call that out. > > The > > + * reference must remain valid until the corresponding > > + * netif_ctrl_response_t has been processed. > > May the ref be r/o? If so, should that be encouraged? > Yes, it can and should be r/o. > > + * > > + * type = NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING: > > + * > > + * This is sent by the frontend to set the mapping of toeplitz hash to > > + * queue number to be applied by the backend. > > + * > > + * The data[0] field is set to the order of the mapping. The data[1] field > > + * is set to a grant reference of a page containing the mapping. The > > + * reference must remain valid until the corresponding > > + * netif_ctrl_response_t has been processed. > > + * > > + * The format of the mapping is: > > + * > > + * 0 1 2 3 4 5 6 7 octet > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | queue[0] | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | queue[1] | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | queue[2] | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | queue[3] | > > + * . > > + * . > > + * | queue[N-1] | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * > > + * where each queue value is less than "multi-queue-num-queues" (see > above) > > + * and N is 1 << data[0]. > > + * > > + * NOTE: Before a specific mapping is set using this request, the backend > > + * should map all toeplitz hash values to queue 0 (which is the only > > + * queue guaranteed to exist in all cases). > > i.e. between receiving a NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS with a non- > zero > set of flags and NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING it should do > this? > Yes. The default map should be zeroed. > If it has not seen a NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS, or it has seen > one > with flags = 0 then as before the b/e is free to steer as it likes? > Yes. No change to default behaviour. > Is it valid to send this mapping command first before the flags? > As explained above, yes. > > > + */ > > + > > +/* > > + * 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. > > > + */ > > + > > +struct netif_ctrl_response { > > + uint16_t id; > > + uint16_t pad; > > + uint32_t status; > > + > > +#define NETIF_CTRL_STATUS_SUCCESS 0 > > +#define NETIF_CTRL_STATUS_NOT_SUPPORTED 1 > > +#define NETIF_CTRL_STATUS_INVALID_PARAMETER 2 > > +#define NETIF_CTRL_STATUS_BUFFER_OVERFLOW 3 > > Is the intention that these will be the union of all possible failure > modes, rather than making provision for type-specific failures? > Yes, they are just supposed to be errno-like things. > These are all pretty generic though, and I suppose we could always figure > that out when some command has a very obscure failure case. > Yes. > > + > > + uint32_t data; > > +}; > > +typedef struct netif_ctrl_response netif_ctrl_response_t; > > + > > +/* > > + * type = > > + * > > + * The default response for any unrecognised request has the status field > > + * set to NETIF_CTRL_STATUS_NOT_SUPPORTED and the data field set to > 0. > > + * > > + * type = NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS: > > + * > > + * Since the request carries no data there is no reason for processing to > > + * fail, hence the status field is set to NETIF_CTRL_STATUS_SUCCESS and > the > > + * data field is a bitwise OR of NETIF_CTRL_TOEPLITZ_FLAG_* values > (defined > > + * above) indicating which hash types are supported by the backend. > > + * If no hashing is supported then the data field should be set to 0. > > I wonder if it would be clearer to define the generic request and response > structures up front followed by the specific commands and their > request+response formats at the same time, rather than splitting the > specific command requests and responses into different places? > > Like: > > Command: NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS > > Description: Blah blah > > Request data format: Words words > Valid response status codes: > - One > - Another > Responds data format: Words > > (i.e. more like API than protocol documentation in the form of independent > looking things going back and forth) > Yes, that may be easier to read. I'll do that. > > + * > > + * 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. > > + * 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 :-) > > > + * 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 requset > should > > "request" > Thanks for all the typo/spelling corrections. I'll pick those up too. Paul > Ian.
> -----Original Message----- > From: David Vrabel [mailto:david.vrabel@citrix.com] > Sent: 08 January 2016 16:07 > To: Paul Durrant; xen-devel@lists.xenproject.org > Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Jan Beulich > Subject: Re: [Xen-devel] [PATCH v4 2/3] public/io/netif.h: document control > ring and toeplitz hashing > > On 07/01/16 13:05, Paul Durrant wrote: > > This patch documents a new shared ring between frontend and backend > that > > can be used to pass bulk out-of-band data, such as that required to > > implement toeplitz hashing in the backend such that it is configurable by > > the frontend. > > > > The patch then goes on to document the messages passed over the control > > ring that can be used to configure toeplitz hashing. > [...] > > --- a/xen/include/public/io/netif.h > > +++ b/xen/include/public/io/netif.h > > @@ -151,6 +151,270 @@ > > */ > > > > /* > > + * Control ring > > + * ============ > [...] > > + * The frontend provides a control ring to the backend by setting: > > + * > > + * /local/domain/<domid>/device/vif/<vif>/ctrl-ring-ref = <gref> > > + * /local/domain/<domid>/device/vif/<vif>/event-channel-ctrl = <port> > > ctrl-event-channel to match ctrl-ring-ref? > I was following the style of the tx and rx rings where it is 'blah-ring-ref' and 'event-channel-blah'. Do you think I should break precedent? Paul > David
On 08/01/16 16:21, Paul Durrant wrote: >> -----Original Message----- >> From: David Vrabel [mailto:david.vrabel@citrix.com] >> Sent: 08 January 2016 16:07 >> To: Paul Durrant; xen-devel@lists.xenproject.org >> Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Jan Beulich >> Subject: Re: [Xen-devel] [PATCH v4 2/3] public/io/netif.h: document control >> ring and toeplitz hashing >> >> On 07/01/16 13:05, Paul Durrant wrote: >>> This patch documents a new shared ring between frontend and backend >> that >>> can be used to pass bulk out-of-band data, such as that required to >>> implement toeplitz hashing in the backend such that it is configurable by >>> the frontend. >>> >>> The patch then goes on to document the messages passed over the control >>> ring that can be used to configure toeplitz hashing. >> [...] >>> --- a/xen/include/public/io/netif.h >>> +++ b/xen/include/public/io/netif.h >>> @@ -151,6 +151,270 @@ >>> */ >>> >>> /* >>> + * Control ring >>> + * ============ >> [...] >>> + * The frontend provides a control ring to the backend by setting: >>> + * >>> + * /local/domain/<domid>/device/vif/<vif>/ctrl-ring-ref = <gref> >>> + * /local/domain/<domid>/device/vif/<vif>/event-channel-ctrl = <port> >> >> ctrl-event-channel to match ctrl-ring-ref? >> > > I was following the style of the tx and rx rings where it is 'blah-ring-ref' and 'event-channel-blah'. Do you think I should break precedent? I hadn't noticed that. Either way is fine by me then. David
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.
> -----Original Message----- > From: Ian Campbell [mailto:ian.campbell@citrix.com] > Sent: 08 January 2016 16:46 > To: Paul Durrant; xen-devel@lists.xenproject.org > Cc: Ian Jackson; Jan Beulich; Keir (Xen.org); Tim (Xen.org) > Subject: Re: [PATCH v4 2/3] public/io/netif.h: document control ring and > toeplitz hashing > > 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. > Sure. Will do. > > > > > > > > > > + */ > > > > + > > > > +/* > > > > + * 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. > I think it should be. > + * > > > > + * 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. > That was the idea. It's actually more straightforward to implement that way. I'll add comments to clarify though. > 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)? Good, point. I'll say that a default key of all zeroes (which means all hash values will be 0) should be assumed until one is set. > > > > > +/* > > > > + * 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? It's pretty fundamental. You need to get back at the grefs used for the key and the mapping so any implementation is pretty certain to have the req in hand. > > > > > > + * > > > > + * 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? > At the moment, it goes into the 'left most' (in Microsoft's terminology) byte and any remaining bytes are zero filled. From the spec it's not clear if zero-filling is the right thing to do but since Windows always sets a 40 byte key it's never been an issue. > > > > > > + * 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. The hash is simply used as an index modulo mapping size (I'll make that clear somewhere) so a mapping size less than the number of queues you have is ok but clearly suboptimal. > > Since this is a single page, what happens with >=512 queues? (and how far > away is that possibility). > Good question. An 8 byte queue number is clearly overkill if we can only steer to 512 of them. I could define a limit on the number of queues at 256 and use byte array instead. Realistically, it's going to be a while before we get to 256 cpus in a Windows VM. Paul > Ian.
On Fri, 2016-01-08 at 17:07 +0000, Paul Durrant wrote: > > +/* > > > > > + * 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? > > It's pretty fundamental. You need to get back at the grefs used for the > key and the mapping so any implementation is pretty certain to have the > req in hand. Not all commands use a gref though, and future ones may not. Given we are otherwise just wasting those 16 bits we may as well stick the type into them. > > > > > > + * > > > > > + * 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? > > > > At the moment, it goes into the 'left most' (in Microsoft's terminology) > byte and any remaining bytes are zero filled. From the spec it's not > clear if zero-filling is the right thing to do but since Windows always > sets a 40 byte key it's never been an issue. I think we should either clearly define the behaviour, or set a minimum size. Does this zero extending apply to any multiple of something other than 32 bits? Like if I give 9 octets does it effectively get padded to 12 with zeroes? > > > > > > > > > + * 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. > > The hash is simply used as an index modulo mapping size (I'll make that > clear somewhere) thanks. > so a mapping size less than the number of queues you have is ok but > clearly suboptimal. > > > > > Since this is a single page, what happens with >=512 queues? (and how > > far > > away is that possibility). > > > > Good question. An 8 byte queue number 8 bytes is the hash match isn't it and Q number is the offset into the page, isn't it? Or have I misunderstood how something fits together? > is clearly overkill if we can only steer to 512 of them. I could define a > limit on the number of queues at 256 and use byte array instead. > Realistically, it's going to be a while before we get to 256 cpus in a > Windows VM. Them's brave words ;-) We should maybe consider that a f/e OS other than Windows might, for some reason, implement Toeplitz and have a higher probability of supporting large #s of vcpus sooner? Split data[0] into (total) size and offset (to set blocks of values)? > Paul > > > Ian. >
> -----Original Message----- > From: Ian Campbell [mailto:ian.campbell@citrix.com] > Sent: 08 January 2016 17:22 > To: Paul Durrant; xen-devel@lists.xenproject.org > Cc: Ian Jackson; Jan Beulich; Keir (Xen.org); Tim (Xen.org) > Subject: Re: [PATCH v4 2/3] public/io/netif.h: document control ring and > toeplitz hashing > > On Fri, 2016-01-08 at 17:07 +0000, Paul Durrant wrote: > > > +/* > > > > > > + * 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? > > > > It's pretty fundamental. You need to get back at the grefs used for the > > key and the mapping so any implementation is pretty certain to have the > > req in hand. > > Not all commands use a gref though, and future ones may not. > > Given we are otherwise just wasting those 16 bits we may as well stick the > type into them. > Ok. Will revert to doing that. > > > > > > > > + * > > > > > > + * 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? > > > > > > > At the moment, it goes into the 'left most' (in Microsoft's terminology) > > byte and any remaining bytes are zero filled. From the spec it's not > > clear if zero-filling is the right thing to do but since Windows always > > sets a 40 byte key it's never been an issue. > > I think we should either clearly define the behaviour, or set a minimum > size. Ok. I may as well set the minimum size at 40 then. > > Does this zero extending apply to any multiple of something other than 32 > bits? Like if I give 9 octets does it effectively get padded to 12 with > zeroes? > No, the key is essentially treated as a shift register so it's just implemented as a byte array. > > > > > > > > > > > > + * 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. > > > > The hash is simply used as an index modulo mapping size (I'll make that > > clear somewhere) > > thanks. > > > so a mapping size less than the number of queues you have is ok but > > clearly suboptimal. > > > > > > > > Since this is a single page, what happens with >=512 queues? (and how > > > far > > > away is that possibility). > > > > > > > Good question. An 8 byte queue number > > 8 bytes is the hash match isn't it and Q number is the offset into the > page, isn't it? Or have I misunderstood how something fits together? No. Other way round :-) The hash is treated as an index into the table, modulo table size and the values in the table are the actual queue numbers. > > > is clearly overkill if we can only steer to 512 of them. I could define a > > limit on the number of queues at 256 and use byte array instead. > > Realistically, it's going to be a while before we get to 256 cpus in a > > Windows VM. > > Them's brave words ;-) > Getting to 32 has been hard enough. Breaking through the 64 cpu limit is going to be 'interesting' too, I suspect. (A lot of core Windows stuff uses native word size bit masks for cpu affinity so you have to start using 'processor groups' and it all gets a bit funky). > We should maybe consider that a f/e OS other than Windows might, for > some > reason, implement Toeplitz and have a higher probability of supporting > large #s of vcpus sooner? > > Split data[0] into (total) size and offset (to set blocks of values)? Ok. If I go for a 16-bit queue number, that's probably enough, and means that a 4k page can hold 2k mappings. Then I'll do as you suggest and use a size, offset pair in place of the order field. Paul > > > Paul > > > > > Ian. > >
diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h index 1790ea0..06e0b61 100644 --- a/xen/include/public/io/netif.h +++ b/xen/include/public/io/netif.h @@ -151,6 +151,270 @@ */ /* + * Control ring + * ============ + * + * Some features, such as toeplitz hashing (detailed below), require a + * significant amount of out-of-band data to be passed from frontend to + * backend. Use of xenstore is not suitable for large quantities of data + * because of quota limitations and so a dedicated 'control ring' is used. + * The ability of the backend to use a control ring is advertised by + * setting: + * + * /local/domain/X/backend/<domid>/<vif>/feature-control-ring = "1" + * + * The frontend provides a control ring to the backend by setting: + * + * /local/domain/<domid>/device/vif/<vif>/ctrl-ring-ref = <gref> + * /local/domain/<domid>/device/vif/<vif>/event-channel-ctrl = <port> + * + * where <gref> is the grant reference of the shared page used to + * implement the control ring and <port> is an event channel to be used + * as a mailbox interrupt, before the frontend moves into the connected + * state. + * + * 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. + */ + +/* + * 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] + * Result = ToeplitzHash(Buffer, 8) + */ +#define _NETIF_CTRL_TOEPLITZ_FLAG_IPV4 0 +#define NETIF_CTRL_TOEPLITZ_FLAG_IPV4 (1 << _NETIF_CTRL_TOEPLITZ_FLAG_IPV4) + +/* + * A hash calculated over an IP version 4 header and TCP header as + * follows: + * + * Buffer[0..12] = Packet[12..15] + Packet[16..19] + + * Packet[20..21] + Packet[22..23] + * Result = ToeplitzHash(Buffer, 12) + */ +#define _NETIF_CTRL_TOEPLITZ_FLAG_IPV4_TCP 1 +#define NETIF_CTRL_TOEPLITZ_FLAG_IPV4_TCP (1 << _NETIF_CTRL_TOEPLITZ_FLAG_IPV4_TCP) + +/* + * A hash calculated over an IP version 6 header as follows: + * + * Buffer[0..32] = Packet[8..23] + Packet[24..39] + * Result = ToeplitzHash(Buffer, 32) + */ +#define _NETIF_CTRL_TOEPLITZ_FLAG_IPV6 2 +#define NETIF_CTRL_TOEPLITZ_FLAG_IPV6 (1 << _NETIF_CTRL_TOEPLITZ_FLAG_IPV4) + +/* + * A hash calculated over an IP version 6 header and TCP header as + * follows: + * + * Buffer[0..36] = Packet[8..23] + Packet[24..39] + + * Packet[40..41] + Packet[42..43] + * Result = ToeplitzHash(Buffer, 36) + */ +#define _NETIF_CTRL_TOEPLITZ_FLAG_IPV6_TCP 3 +#define NETIF_CTRL_TOEPLITZ_FLAG_IPV6_TCP (1 << _NETIF_CTRL_TOEPLITZ_FLAG_IPV4_TCP) + +/* + * Control requests (netif_ctrl_request_t) + * ======================================= + * + * All requests have the following format: + * + * 0 1 2 3 4 5 6 7 octet + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | id | type | data[0] | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | data[1] | + * +-----+-----+-----+-----+ + * + * id: the request identifier, echoed in response. + * type: the type of request (see below) + * data[]: any data associated with the request (determined by type) + */ + +struct netif_ctrl_request { + uint16_t id; + uint16_t type; + +#define NETIF_CTRL_TYPE_INVALID 0 +#define NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS 1 +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_FLAGS 2 +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_KEY 3 +#define NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING 4 + + uint32_t data[2]; +}; +typedef struct netif_ctrl_request netif_ctrl_request_t; + +/* + * type = NETIF_CTRL_TYPE_GET_TOEPLITZ_FLAGS: + * + * This is sent by the frontend to query the types of toeplitz + * hash supported by the backend. No data is required and to the + * data[] field is set to 0. + * + * 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: + * + * This is sent by the frontend to set the key of toeplitz hash that + * the backend should calculate. The toeplitz algorithm is illustrated + * by the following pseudo-code: + * + * (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 data[0] field is set to the size of key in octets. The data[1] + * field is set to a grant reference of a page containing the key. The + * reference must remain valid until the corresponding + * netif_ctrl_response_t has been processed. + * + * type = NETIF_CTRL_TYPE_SET_TOEPLITZ_MAPPING: + * + * This is sent by the frontend to set the mapping of toeplitz hash to + * queue number to be applied by the backend. + * + * The data[0] field is set to the order of the mapping. The data[1] field + * is set to a grant reference of a page containing the mapping. The + * reference must remain valid until the corresponding + * netif_ctrl_response_t has been processed. + * + * The format of the mapping is: + * + * 0 1 2 3 4 5 6 7 octet + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | queue[0] | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | queue[1] | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | queue[2] | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | queue[3] | + * . + * . + * | queue[N-1] | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * + * where each queue value is less than "multi-queue-num-queues" (see above) + * and N is 1 << data[0]. + * + * NOTE: Before a specific mapping is set using this request, the backend + * should map all toeplitz hash values to queue 0 (which is the only + * queue guaranteed to exist in all cases). + */ + +/* + * 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) + */ + +struct netif_ctrl_response { + uint16_t id; + uint16_t pad; + uint32_t status; + +#define NETIF_CTRL_STATUS_SUCCESS 0 +#define NETIF_CTRL_STATUS_NOT_SUPPORTED 1 +#define NETIF_CTRL_STATUS_INVALID_PARAMETER 2 +#define NETIF_CTRL_STATUS_BUFFER_OVERFLOW 3 + + uint32_t data; +}; +typedef struct netif_ctrl_response netif_ctrl_response_t; + +/* + * type = <unknown> + * + * The default response for any unrecognised request has the status field + * set to NETIF_CTRL_STATUS_NOT_SUPPORTED and the data field set to 0. + * + * type = NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS: + * + * Since the request carries no data there is no reason for processing to + * fail, hence the status field is set to NETIF_CTRL_STATUS_SUCCESS and the + * data field is a bitwise OR of NETIF_CTRL_TOEPLITZ_FLAG_* values (defined + * above) indicating which hash types are supported by the backend. + * If no hashing is supported then the data field should be set to 0. + * + * 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 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) + * 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) + * 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 requset should + * succeed and hence the status field is set to NETIF_CTRL_STATUS_SUCCESS. + * The data field should be set to 0. + */ + +DEFINE_RING_TYPES(netif_ctrl, struct netif_ctrl_request, struct netif_ctrl_response); + +/* * Guest transmit * ============== *
This patch documents a new shared ring between frontend and backend that can be used to pass bulk out-of-band data, such as that required to implement toeplitz hashing in the backend such that it is configurable by the frontend. The patch then goes on to document the messages passed over the control ring that can be used to configure toeplitz hashing. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> Cc: Tim Deegan <tim@xen.org> --- v4: - Fix netif_ctrl_response_t definition to match specification v3: - Fix commit comment v2: - Use a balanced fix-sized message ring for the control ring (bulk data now passed by grant reference). --- xen/include/public/io/netif.h | 264 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 264 insertions(+)