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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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
> > > > > > > @@ -123,6 +123,42 @@ typedef struct xen_argo_unregister_ring
> > > > > > >  /* Messages on the ring are padded to a multiple of this size. */
> > > > > > >  #define XEN_ARGO_MSG_SLOT_SIZE 0x10
> > > > > > >
> > > > > > > +/*
> > > > > > > + * Notify flags
> > > > > > > + */
> > > > > > > +/* Ring is empty */
> > > > > > > +#define XEN_ARGO_RING_DATA_F_EMPTY       (1U << 0)
> > > > > > > +/* Ring exists */
> > > > > > > +#define XEN_ARGO_RING_DATA_F_EXISTS      (1U << 1)
> > > > > > > +/* Pending interrupt exists. Do not rely on this field - for 
> > > > > > > profiling only */
> > > > > > > +#define XEN_ARGO_RING_DATA_F_PENDING     (1U << 2)
> > > >
> > > > Regarding this flag, I've just noticed while looking at the code that
> > > > it doesn't seem to relate to interrupts?
> > >
> > > It might not seem that way, but I think it does, because it indicates
> > > that the hypervisor has just queued up a signal (via VIRQ) for later:
> > > the logic in fill_ring_data has observed that there wasn't enough
> > > space available in the ring for the requested space_required supplied
> > > in the notify call, so it has added a new entry to the ring's
> > > pending_ent list, which will cause a signal to be triggered to the
> > > domain (ie. a VIRQ) later when enough space has been observed as being
> > > available.
> >
> > Oh, I think I was getting confused by the wording of the comment, here
> > "pending interrupt" means that the caller should expect an interrupt at
> > some point in the future when there's enough free space on the ring?
> 
> Yes, that's right.
> 
> > To me "pending interrupt" means there's an interrupt set by the
> > hypervisor which has not yet been serviced by the caller.
> 
> OK, I could see that is a reasonable interpretation too. Do you have a
> term that you would prefer for this?

My proposal was 'notify', but I'm quite bad at naming things TBH.

> >
> > > Now, the "len" value stored in that pending_ent can be changed later,
> > > depending on the size of messages that the domain attempts to send to
> > > the same ring in the meantime, which I think is why the comment notes
> > > not to depend upon that flag.
> > >
> > > > From it's usage in fill_ring_data I would write the following
> > > > description:
> > > >
> > > > "Likely not enough space to queue a message of `space_required`
> > > > size."
> > > >
> > > > And then XEN_ARGO_RING_DATA_F_PENDING is completely orthogonal to
> > > > XEN_ARGO_RING_DATA_F_SUFFICIENT, at which point having only one of
> > > > those would be enough?
> > >
> > > Given the above, where I do think that the PENDING flag is an
> > > indicator of queued interrupt, I think there's some merit to keeping
> > > them separate, rather than committing to the client that it is always
> > > one or the other. It actually looks like the call to pending_requeue
> > > is ignoring the potential for an error value (eg ENOSPC or ENOMEM)
> > > there, where the flag should not be set, and possibly the errno should
> > > be returned to the caller.
> >
> > Yes, you should propagate the errors from pending_requeue to the
> > caller.
> 
> ack, done.
> 
> >
> > > > 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?
> > >
> > > right, but there is a case where you can get one with neither bit set.
> >
> > Yes, that's right. But you would then get the
> > XEN_ARGO_RING_DATA_F_EMSGSIZE flag set or the ring simply don't
> > exist.
> >
> > > It looks a bit clearer for the caller to have the explicit separate
> > > bits because it can avoid having to check a third flag first to see
> > > how to interpret a combined one.
> >
> > 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?

In fact I think XEN_ARGO_RING_EMSGSIZE could be removed also, and the
hypercall made return E2BIG?

> 
> > 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)
> >
> > Note that I've also removed the _DATA_F_, I think it's not specially
> > helpful, and shorter names are easier to read.
> 
> Ack - done.
> 
> >
> > 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.
> 
> ok -- let me know your view given the description of Situation 4 above.
> I've kept it unchanged for the time being.

As said in my previous reply, my comments where recommendations but I
don't have a strong opinion because I don't know the users of the
interface. My PoV is that adding flags for errors seems like
duplicating error codes, and duplicating the places the caller has to
check for errors (in your proposal return value from hypercall and
flags field can both contain errors).

For example if the message is bigger than the ring size, the hypercall
is going to set the XEN_ARGO_RING_EMSGSIZE flag and return 0? I think
you could avoid the XEN_ARGO_RING_EMSGSIZE flag and just make the
hypercall return E2BIG.

Thanks, Roger.

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