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

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

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
> > > > > @@ -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?

To me "pending interrupt" means there's an interrupt set by the
hypervisor which has not yet been serviced by the caller.

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

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

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.

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.

Thanks, Roger.

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