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

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Sat, Jan 19, 2019 at 4:06 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx> 
> > 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@xxxxxxxxxx> 
> > > > 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@xxxxxxxxxx> 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.

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

Christopher

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