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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> Sent: 18 September 2017 12:56
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Xen-devel <xen-devel@xxxxxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Subject: Re: [PATCH v3 1/1] public/io/netif.h: add gref mapping control
> messages
> 
> 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.

The 'all or nothing' semantic is easier for the frontend to deal with, so I 
think that's the way to go. Otherwise you'd need the per-entry status, as you 
say. Either way, I don't think the data return is particularly useful.

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

I think that last sentence might be better as something like:

'The entries used are only the ones representing grant references that were 
previously the subject of a XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING operation. Any 
other entries will have their status set to 
XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER upon completion.'

Does that sound ok?

Cheers,

   Paul

> 
> Thank you!

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