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

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

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?

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

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

Christopher

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