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

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

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.

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

Thanks, Roger.

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