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

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

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.

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.

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

>
> > > > +/* Sufficient space to queue space_required bytes exists */
> > > > +#define XEN_ARGO_RING_DATA_F_SUFFICIENT  (1U << 3)
> > >
> > > I would reword this as:
> > >
> > > "Sufficient space to queue space_required bytes might exists"
> > >
> > > Because AFAICT as soon as the hypervisor drops the L3 lock the space
> > > available might change, so the recipient of the notification or the
> > > return from the hypercall shouldn't expect that there _must_ be
> > > space_required available space on the ring.
> >
> > ack. does this look ok? -:
> >
> > + * Sufficient space to queue space_required bytes has become available.
> > + * If messages have been sent, it may not still be available.
>
> I think my suggestion was shorter and clearer, but I'm not a native
> speaker so if you think the above is better and no one else complains
> that's fine for me.

ok, Jan's edit to yours looks good and a single line is nicer so:
"Sufficient space to queue space_required bytes might exist"

Christopher

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