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

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

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:
> > Queries for data about space availability in registered rings and
> > causes notification to be sent when space has become available.
> >
> > The hypercall op populates a supplied data structure with information about
> > ring state, and if insufficient space is currently available in a given 
> > ring,
> > the hypervisor will record the domain's expressed interest and notify it
> > when it observes that space has become available.
> >
> > Checks for free space occur when this notify op is invoked, so it may be
> > intentionally invoked with no data structure to populate
> > (ie. a NULL argument) to trigger such a check and consequent notifications.
> >
> > Limit the maximum number of notify requests in a single operation to a
> > simple fixed limit of 256.
>
> LGTM, I have one comment on the public interface, the other comment is
> purely cosmetic.
>
> >  static int
> > +fill_ring_data(const struct domain *currd,
> > +               XEN_GUEST_HANDLE(xen_argo_ring_data_ent_t) data_ent_hnd)
> > +{
> > +    xen_argo_ring_data_ent_t ent;
> > +    struct domain *dst_d;
> > +    struct argo_ring_info *ring_info;
> > +
> > +    ASSERT(currd == current->domain);
> > +    ASSERT(LOCKING_Read_L1);
> > +
> > +    if ( __copy_from_guest(&ent, data_ent_hnd, 1) )
> > +        return -EFAULT;
> > +
> > +    argo_dprintk("fill_ring_data: ent.ring.domain=%u,ent.ring.aport=%x\n",
> > +                 ent.ring.domain_id, ent.ring.aport);
> > +
> > +    ent.flags = 0;
> > +
> > +    dst_d = get_domain_by_id(ent.ring.domain_id);
> > +    if ( dst_d )
> > +    {
> > +        if ( dst_d->argo )
> > +        {
> > +            read_lock(&dst_d->argo->rings_L2_rwlock);
> > +
> > +            ring_info = find_ring_info_by_match(dst_d, ent.ring.aport,
> > +                                                currd->domain_id);
> > +            if ( ring_info )
> > +            {
>
> Nit: there's a lot of nested conditions here, which push the
> indentation to the right. It might be better from a readability PoV to
> return early. For example:
>
> if ( !dst_d || !dst_d->argo)
>     goto out;
>
> ...
>
> if ( !ring_info )
> {
>     read_unlock(&dst_d->argo->rings_L2_rwlock);
>     goto out;
> }
>
> ...
>
> out:
> if ( dst_d )
>     put_domain(dst_d);
>
> if ( copy_to_....
>
> In order to prevent so much space wastage due to indentation.

Thanks, yes - done.

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

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