Re: [Xen-devel] [PATCH 16/25] argo: implement the notify op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.18 at 02:32, <christopher.w.clark@xxxxxxxxx> wrote:
> +static uint32_t
> +argo_ringbuf_payload_space(struct domain *d, struct argo_ring_info 
> *ring_info)
> +{
> +    argo_ring_t ring;
> +    int32_t ret;
> +
> +    ASSERT(spin_is_locked(&ring_info->lock));
> +
> +    ring.len = ring_info->len;
> +    if ( !ring.len )
> +        return 0;
> +
> +    ring.tx_ptr = ring_info->tx_ptr;
> +
> +    if ( argo_ringbuf_get_rx_ptr(ring_info, &ring.rx_ptr) )
> +        return 0;
> +
> +    argo_dprintk("argo_ringbuf_payload_space: tx_ptr=%d rx_ptr=%d\n",
> +                 ring.tx_ptr, ring.rx_ptr);
> +
> +    if ( ring.rx_ptr == ring.tx_ptr )
> +        return ring.len - sizeof(struct argo_ring_message_header);
> +
> +    ret = ring.rx_ptr - ring.tx_ptr;
> +    if ( ret < 0 )
> +        ret += ring.len;

Seeing these two if()-s - how is an empty ring distinguished from
a completely full one? I'm getting the impression that
ring.rx_ptr == ring.tx_ptr in both cases.

> +    ret -= sizeof(struct argo_ring_message_header);
> +    ret -= ARGO_ROUNDUP(1);

Wouldn't you instead better round ret to a suitable multiple of
whatever granularity you try to arrange for here? Otherwise
what is this extra subtraction supposed to do?

> @@ -627,6 +679,43 @@ argo_pending_remove_all(struct argo_ring_info *ring_info)
>      }
>  }
>  
> +static void
> +argo_pending_notify(struct hlist_head *to_notify)
> +{
> +    struct hlist_node *node, *next;
> +    struct argo_pending_ent *pending_ent;
> +
> +    ASSERT(rw_is_locked(&argo_lock));
> +
> +    hlist_for_each_entry_safe(pending_ent, node, next, to_notify, node)
> +    {
> +        hlist_del(&pending_ent->node);
> +        argo_signal_domid(pending_ent->id);
> +        xfree(pending_ent);
> +    }
> +}
> +
> +static void
> +argo_pending_find(const struct domain *d, struct argo_ring_info *ring_info,
> +                  uint32_t payload_space, struct hlist_head *to_notify)
> +{
> +    struct hlist_node *node, *next;
> +    struct argo_pending_ent *ent;
> +
> +    ASSERT(rw_is_locked(&d->argo->lock));
> +
> +    spin_lock(&ring_info->lock);
> +    hlist_for_each_entry_safe(ent, node, next, &ring_info->pending, node)
> +    {
> +        if ( payload_space >= ent->len )
> +        {
> +            hlist_del(&ent->node);
> +            hlist_add_head(&ent->node, to_notify);
> +        }
> +    }

So if there's space available to fit e.g. just the first pending entry,
you'd continue the loop and also signal all others, provided their
lengths aren't too big? What good does producing such a burst of
notifications do, when only one of the interested parties is
actually going to be able to put something on the ring?

> @@ -705,6 +812,107 @@ argo_ring_remove_info(struct domain *d, struct 
> argo_ring_info *ring_info)
>      xfree(ring_info);
>  }
>  
> +/*ring data*/

Now this comment is malformed in any event.

> +static int
> +argo_fill_ring_data(struct domain *src_d,

const?

> +static long
> +argo_notify(struct domain *d,
> +            XEN_GUEST_HANDLE_PARAM(argo_ring_data_t) ring_data_hnd)
> +{
> +    argo_ring_data_t ring_data;
> +    int ret = 0;
> +
> +    read_lock(&argo_lock);
> +
> +    if ( !d->argo )
> +    {
> +        read_unlock(&argo_lock);
> +        argo_dprintk("!d->argo, ENODEV\n");
> +        return -ENODEV;
> +    }
> +
> +    argo_notify_check_pending(d);
> +
> +    do {
> +        if ( !guest_handle_is_null(ring_data_hnd) )
> +        {
> +            /* Quick sanity check on ring_data_hnd */
> +            ret = copy_field_from_guest_errno(&ring_data, ring_data_hnd, 
> magic);
> +            if ( ret )
> +                break;
> +
> +            if ( ring_data.magic != ARGO_RING_DATA_MAGIC )
> +            {
> +                argo_dprintk(
> +                    "ring.magic(%"PRIx64") != ARGO_RING_MAGIC(%llx), 
> EINVAL\n",
> +                    ring_data.magic, ARGO_RING_MAGIC);
> +                ret = -EINVAL;
> +                break;
> +            }
> +
> +            ret = copy_from_guest_errno(&ring_data, ring_data_hnd, 1);
> +            if ( ret )
> +                break;
> +
> +            {
> +                /*
> +                 * This is a guest pointer passed as a field in a struct
> +                 * so XEN_GUEST_HANDLE is used.
> +                 */
> +                XEN_GUEST_HANDLE(argo_ring_data_ent_t) ring_data_ent_hnd;
> +                ring_data_ent_hnd = guest_handle_for_field(ring_data_hnd,
> +                                                           
> argo_ring_data_ent_t,
> +                                                           data[0]);
> +                ret = argo_fill_ring_data_array(d, ring_data.nent,
> +                                                ring_data_ent_hnd);
> +            }

Stray braces and bogus indentation: The declaration can move up
and then you don't need the braces and the extra level of indentation.

> @@ -103,6 +104,40 @@ typedef struct argo_ring
>   */
>  #define ARGO_ROUNDUP(a) (((a) + 0xf) & ~(typeof(a))0xf)
>  
> +/*
> + * Notify flags
> + */
> +/* Ring is empty */
> +#define ARGO_RING_DATA_F_EMPTY       (1U << 0)
> +/* Ring exists */
> +#define ARGO_RING_DATA_F_EXISTS      (1U << 1)
> +/* Pending interrupt exists. Do not rely on this field - for profiling only 
> */
> +#define ARGO_RING_DATA_F_PENDING     (1U << 2)
> +/* Sufficient space to queue space_required bytes exists */
> +#define ARGO_RING_DATA_F_SUFFICIENT  (1U << 3)
> +
> +typedef struct argo_ring_data_ent
> +{
> +    argo_addr_t ring;
> +    uint16_t flags;
> +    uint16_t pad;
> +    uint32_t space_required;
> +    uint32_t max_message_size;
> +} argo_ring_data_ent_t;
> +
> +typedef struct argo_ring_data
> +{
> +    uint64_t magic;

What is this good for?

> @@ -179,6 +214,33 @@ struct argo_ring_message_header
>   */
>  #define ARGO_MESSAGE_OP_sendv               5
>  
> +/*
> + * ARGO_MESSAGE_OP_notify
> + *
> + * Asks Xen for information about other rings in the system.
> + *
> + * ent->ring is the argo_addr_t of the ring you want information on.
> + * Uses the same ring matching rules as ARGO_MESSAGE_OP_sendv.
> + *
> + * ent->space_required : if this field is not null then Xen will check
> + * that there is space in the destination ring for this many bytes of  
> payload.
> + * If sufficient space is available, it will set ARGO_RING_DATA_F_SUFFICIENT
> + * and CANCEL any pending notification for that ent->ring; otherwise it
> + * will schedule a notification event and the flag will not be set.
> + *
> + * These flags are set by Xen when notify replies:
> + * ARGO_RING_DATA_F_EMPTY       ring is empty
> + * ARGO_RING_DATA_F_PENDING     notify event is pending - * don't rely on 
> this *
> + * ARGO_RING_DATA_F_SUFFICIENT  sufficient space for space_required is there
> + * ARGO_RING_DATA_F_EXISTS      ring exists
> + *
> + * arg1: XEN_GUEST_HANDLE(argo_ring_data_t) ring_data (may be NULL)
> + * arg2: NULL
> + * arg3: 0 (ZERO)
> + * arg4: 0 (ZERO)

Another observation I probably should have made earlier: You
don't check that the NULL/ZERO specified argument are indeed
so. Just like for padding fields, please do.

Jan


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