Re: [Xen-devel] [PATCH v3 1/1] public/io/netif.h: add gref mapping control messages

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On Mon, Sep 18, 2017 at 09:53:18AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > Sent: 13 September 2017 19:11
> > To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>;
> > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Joao Martins
> > <joao.m.martins@xxxxxxxxxx>
> > Subject: [PATCH v3 1/1] public/io/netif.h: add gref mapping control messages
> > 
> > Adds 3 messages to allow guest to let backend keep grants mapped,
> > such that 1) guests allowing fast recycling of pages can avoid doing
> > grant ops for those cases, or otherwise 2) preferring copies over
> > grants and 3) always using a fixed set of pages for network I/O.
> > 
> > The three control ring messages added are:
> >  - Add grefs to be mapped by backend
> >  - Remove grefs mappings (If they are not in use)
> >  - Get maximum amount of grefs kept mapped.
> > 
> > Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> > ---
> > v3:
> > * Use DEL for unmapping grefs instead of PUT
> > * Rname from xen_netif_gref_alloc to xen_netif_gref
> > * Add 'status' field on xen_netif_gref
> > * Clarify what 'inflight' means
> > * Use "beginning of the page" instead of "beginning of the grant"
> > * Mention that page needs to be r/w (as it will have to modify \.status)
> > * `data` on ADD|PUT returns number of entries mapped/unmapped.
> > ---
> >  xen/include/public/io/netif.h | 115
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 115 insertions(+)
> > 
> > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> > index ca0061410d..0080a260fd 100644
> > --- a/xen/include/public/io/netif.h
> > +++ b/xen/include/public/io/netif.h
> > @@ -353,6 +353,9 @@ struct xen_netif_ctrl_request {
> >  #define XEN_NETIF_CTRL_TYPE_SET_HASH_MAPPING_SIZE 5
> >  #define XEN_NETIF_CTRL_TYPE_SET_HASH_MAPPING      6
> >  #define XEN_NETIF_CTRL_TYPE_SET_HASH_ALGORITHM    7
> > +#define XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE 8
> > +#define XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING      9
> > +#define XEN_NETIF_CTRL_TYPE_DEL_GREF_MAPPING     10
> > 
> >      uint32_t data[3];
> >  };
> > @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response {
> >  };
> > 
> >  /*
> > + * Static Grants (struct xen_netif_gref)
> > + * =====================================
> > + *
> > + * A frontend may provide a fixed set of grant references to be mapped on
> > + * the backend. The message of type
> > XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > + * prior its usage in the command ring allows for creation of these 
> > mappings.
> > + * The backend will maintain a fixed amount of these mappings.
> > + *
> > + * XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE lets a frontend
> > query how many
> > + * of these mappings can be kept.
> > + *
> > + * Each entry in the XEN_NETIF_CTRL_TYPE_{ADD,DEL}_GREF_MAPPING
> > input table has
> > + * the following format:
> > + *
> > + *    0     1     2     3     4     5     6     7  octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | grant ref             |  flags    |  status   |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + *
> > + * grant ref: grant reference
> > + * flags: flags describing the control operation
> > + * status: XEN_NETIF_CTRL_STATUS_*
> > + */
> 
> You may want to add some words here pointing out that the status is an
> 'out' field, and also whether it should be initialized to zero or not.
> 
OK.

> > +
> > +struct xen_netif_gref {
> > +       grant_ref_t ref;
> > +       uint16_t flags;
> > +
> > +#define _XEN_NETIF_CTRLF_GREF_readonly    0
> > +#define XEN_NETIF_CTRLF_GREF_readonly
> > (1U<<_XEN_NETIF_CTRLF_GREF_readonly)
> > +
> > +       uint16_t status;
> > +};
> > +
> > +/*
> >   * Control messages
> >   * ================
> >   *
> > @@ -609,6 +647,83 @@ struct xen_netif_ctrl_response {
> >   *       invalidate any table data outside that range.
> >   *       The grant reference may be read-only and must remain valid until
> >   *       the response has been processed.
> > + *
> > + * XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE
> > + * -----------------------------------------
> > + *
> > + * This is sent by the frontend to fetch the number of grefs that can be 
> > kept
> > + * mapped in the backend.
> > + *
> > + * Request:
> > + *
> > + *  type    = XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE
> > + *  data[0] = queue index (assumed 0 for single queue)
> > + *  data[1] = 0
> > + *  data[2] = 0
> > + *
> > + * Response:
> > + *
> > + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation not
> > + *                                                     supported
> > + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - The queue index
> > is
> > + *                                                     out of range
> > + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation successful
> > + *  data   = maximum number of entries allowed in the gref mapping table
> > + *           (if operation was successful) or zero if it is not supported.
> > + *
> > + * XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > + * ------------------------------------
> > + *
> > + * This is sent by the frontend for backend to map a list of grant
> > + * references.
> > + *
> > + * Request:
> > + *
> > + *  type    = XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > + *  data[0] = queue index
> > + *  data[1] = grant reference of page containing the mapping list
> > + *            (r/w and assumed to start at beginning of page)
> > + *  data[2] = size of list in entries
> > + *
> > + * Response:
> > + *
> > + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation not
> > + *                                                     supported
> > + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - Operation failed
> > + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation successful
> > + *  data   = number of entries that were mapped
> > + *
> > + * NOTE: Each entry in the input table has the format outlined
> > + *       in struct xen_netif_gref.
> 
> You may want to put words here about the 'all or nothing' semantics of
> this operation vs. the semantics of the 'del' operation below.
> 
Good point I'll add a paragraph about that.

For the unmap it is clear that status should be per-entry for reasons
discussed on v2. Do you think ADD 'all or nothing' like I had on v2 ?
If so I should remove the 'data' return part since it is not really
useful here.

> > + *
> > + * XEN_NETIF_CTRL_TYPE_DEL_GREF_MAPPING
> > + * ------------------------------------
> > + *
> > + * This is sent by the frontend for backend to unmap a list of grant
> > + * references.
> > + *
> > + * Request:
> > + *
> > + *  type    = XEN_NETIF_CTRL_TYPE_DEL_GREF_MAPPING
> > + *  data[0] = queue index
> > + *  data[1] = grant reference of page containing the mapping list
> > + *            (r/w and assumed to start at beginning of page)
> > + *  data[2] = size of list in entries
> > + *
> > + * Response:
> > + *
> > + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation not
> > + *                                                     supported
> > + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - Operation failed
> > + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation successful
> > + *  data   = number of entries that were unmapped
> > + *
> > + * NOTE: Each entry in the input table has the format outlined in struct
> > + *       xen_netif_gref. The only valid entries are those previously added 
> > with
> > + *       message XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING are valid,
> > otherwise
> > + *       entries status are set to
> > XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER.
> > + *       It will also have the same error for entries inflight i.e. used in
> > + *       requests for which haven't been sent responses to the the 
> > frontend.
> 
> This last paragraph is a bit garbled.

Hmm, sorry for the twisted english. I removed the inflight part which
was making it more confusing and made it clear in the paragraph that the
only valid entries in this message are those added with
ADD_GREF_MAPPING. Hopefully the text below it's better?

NOTE: Each entry in the input table has the format outlined in struct
xen_netif_gref. The mappings hereby used are only the ones previously
added with message XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING. Any other
mappings used here will deliver an
XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER.

Thank you!

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