Re: [PATCH v4 10/14] argo: implement the notify op

["Roger Pau Monné" <roger.pau@citrix.com>]

On Sun, Jan 20, 2019 at 05:59:36PM -0800, Christopher Clark wrote:
> On Sat, Jan 19, 2019 at 4:06 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Fri, Jan 18, 2019 at 03:54:14PM -0800, Christopher Clark wrote:
> > > On Fri, Jan 18, 2019 at 1:44 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > >
> > > > On Thu, Jan 17, 2019 at 01:44:32PM -0800, Christopher Clark wrote:
> > > > > On Thu, Jan 17, 2019 at 3:12 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 16, 2019 at 10:54:48PM -0800, Christopher Clark wrote:
> > > > > > > On Tue, Jan 15, 2019 at 8:19 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jan 15, 2019 at 01:27:42AM -0800, Christopher Clark wrote:
> > > > > > > > > diff --git a/xen/include/public/argo.h b/xen/include/public/argo.h
> > > > > > > > > index c12a50f..d2cb594 100644
> > > > > > > > > --- a/xen/include/public/argo.h
> > > > > > > > > +++ b/xen/include/public/argo.h
> 
> > > > > > AFAICT you cannot get a xen_argo_ring_data_ent_t with both
> > > > > > XEN_ARGO_RING_DATA_F_PENDING and XEN_ARGO_RING_DATA_F_SUFFICIENT set
> > > > > > at the same time?
> > > >
> > > > There are three possible situations, which are mutually exclusive:
> > > >
> > > > 1. Message is bigger than the max message size supported by the ring:
> > > >    set EMSGSIZE
> > > > 2. Message fits based on the current available space on the ring:
> > > >    don't set any flags.
> > > > 3. Message doesn't fit based on the current available space on the
> > > >    ring: set NOTIFY.
> > >
> > > Unfortunately, given the new error checking (added for my "ack, done." above),
> > > now there is a fourth condition. Situation 3 is described more fully as:
> > >
> > > 3. Message doesn't fit based on the current available space on the
> > > ring, and a VIRQ is queued for when space is available: set NOTIFY.
> > >
> > > New Situation 4 is:
> > >
> > > 4. Message doesn't fit based on the current available space on the ring,
> > > but Xen can't queue up a VIRQ for later because memory allocation to
> > > add an entry for that failed. Don't set NOTIFY.
> > >
> > > We ought to enable the guest to distinguish Situation 2 from Situation 4
> > > -- which I think points to keeping the separate flags.
> >
> > But situation 4 is going to return an error code from the hypercall
> > (ENOSPC?), at which point you will be able to differentiate it?
> 
> Ack, ok. Since ENOSPC aborts the return of any further data about that ring,
> (or subsequent rings that were queried in the same op) yes, it's distinct.
> 
> > In fact I think XEN_ARGO_RING_EMSGSIZE could be removed also, and the
> > hypercall made return E2BIG?
> 
> This is the query interface for the sender to a ring to discover the
> receiver's ring size, and it's an interface for querying about multiple
> rings in the same operation; it may not know an individual ring size
> beforehand, or that a given payload size will exceed it.
> 
> Returning E2BIG would abort the loop (in notify, that calls fill_ring_data)
> and not actually return the state to the caller indicating the size of the
> maximum acceptable message size that it needs to avoid that error.
> Instead, we're using the bit in the per-ring response to indicate that
> (non-serious) condition and allowing the loop to continue and provide data
> about all the other rings in the request, including maximum message sizes.

Right, I've been thinking about this and since this is a status
request hypercall it might make sense to return some of what would be
errors (if this was a write to the ring) as flags, and leave the
return error code to be used only for errors that actually prevent the
hypervisor from successfully executing the status hypercall. I leave
up to you to decide what's worth putting in the flags field or
returning as an error code.

> > > > So that would leave the following set of flags:
> > > >
> > > > /* Ring is empty. */
> > > > #define XEN_ARGO_RING_EMPTY       (1U << 0)
> > > > /* Ring exists. */
> > > > #define XEN_ARGO_RING_EXISTS      (1U << 1)
> > > > /*
> > > >  * Not enough ring space available for the requested size, caller set
> > > >  * to receive a notification via VIRQ_ARGO when enough free space
> > > >  * might be available.
> > > >  */
> > > > #define XEN_ARGO_RING_NOTIFY      (1U << 2)
> > > > /* Requested size exceeds maximum ring message size. */
> > > > #define XEN_ARGO_RING_EMSGSIZE    (1U << 3)
> > > > /* Ring is shared, not unicast. */
> > > > #define XEN_ARGO_RING_SHARED      (1U << 4)
> > > >
> > > >
> > > > I think the above is clearer and should be able to convey the
> > > > same set of information using one flag less, which is always better
> > > > IMO. That being set I don't know the users of this interface anyway,
> > > > so if you think the original proposal is better I'm not going to
> > > > oppose.
> 
> I've checked both the Linux (prototype Argo) and Windows (v4v) drivers and
> of the original flags, the SUFFICIENT flag is used, while the PENDING one is
> not, which fits with its description of being for profiling only; so given
> that, I'll keep the SUFFICIENT flag, but will drop the PENDING one, leaving
> it to be inferred from the other state returned, as requested.

Ack, as said above, I leave up to you to decide what flags to use. At
the end of day there are clients already for this interface, so I
assume the flags are functional for the needs of the clients.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel