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

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

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?

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?

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?

> > > +/* 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.

Thanks, Roger.

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