Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

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

On Wed, Jun 12, 2019 at 5:35 AM Nicholas Tsirakis
<niko.tsirakis@xxxxxxxxx> wrote:
>
> When a message is requeue'd in Xen's internal queue, the queue
> entry contains the length of the message so that Xen knows to
> send a VIRQ to the respective domain when enough space frees up
> in the ring. Due to a small bug, however, Xen doesn't populate
> the length of the msg if a given write fails, so this length is
> always reported as zero. This causes Xen to spurriously wake up

I should've caught this earlier and it would be nice to fix on commit:
only a single R in "spuriously"

> a domain even when the ring doesn't have enough space.
>
> This patch makes sure that the msg len is properly reported by
> populating it in the event of a write failure.
>
> Signed-off-by: Nicholas Tsirakis <tsirakisn@xxxxxxxxxxxx>
Reviewed-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>

> ---
>  xen/common/argo.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 2f874a570d..c8f4302963 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -765,27 +765,20 @@ iov_count(const xen_argo_iov_t *piov, unsigned int niov,
>  static int
>  ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info,
>                 const struct argo_ring_id *src_id, xen_argo_iov_t *iovs,
> -               unsigned int niov, uint32_t message_type,
> -               unsigned long *out_len)
> +               unsigned int niov, uint32_t message_type, unsigned int len)
>  {
>      xen_argo_ring_t ring;
>      struct xen_argo_ring_message_header mh = { };
>      int sp, ret;
> -    unsigned int len = 0;
>      xen_argo_iov_t *piov;
>      XEN_GUEST_HANDLE(uint8) NULL_hnd = { };
>
>      ASSERT(LOCKING_L3(d, ring_info));
>
>      /*
> -     * Obtain the total size of data to transmit -- sets the 'len' variable
> -     * -- and sanity check that the iovs conform to size and number limits.
>       * Enforced below: no more than 'len' bytes of guest data
>       * (plus the message header) will be sent in this operation.
>       */
> -    ret = iov_count(iovs, niov, &len);
> -    if ( ret )
> -        return ret;
>
>      /*
>       * Upper bound check the message len against the ring size.
> @@ -983,8 +976,6 @@ ringbuf_insert(const struct domain *d, struct 
> argo_ring_info *ring_info,
>       * versus performance cost could be added to decide that here.
>       */
>
> -    *out_len = len;
> -
>      return ret;
>  }
>
> @@ -1976,7 +1967,7 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>      struct argo_ring_id src_id;
>      struct argo_ring_info *ring_info;
>      int ret = 0;
> -    unsigned long len = 0;
> +    unsigned int len = 0;
>
>      argo_dprintk("sendv: (%u:%x)->(%u:%x) niov:%u type:%x\n",
>                   src_addr->domain_id, src_addr->aport, dst_addr->domain_id,
> @@ -2044,17 +2035,25 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>      {
>          spin_lock(&ring_info->L3_lock);
>
> -        ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
> -                             message_type, &len);
> -        if ( ret == -EAGAIN )
> +        /*
> +         * Obtain the total size of data to transmit -- sets the 'len' 
> variable
> +         * -- and sanity check that the iovs conform to size and number 
> limits.
> +         */
> +        ret = iov_count(iovs, niov, &len);
> +        if ( !ret )
>          {
> -            int rc;
> +            ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
> +                                 message_type, len);
> +            if ( ret == -EAGAIN )
> +            {
> +                int rc;
>
> -            argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
> -            /* requeue to issue a notification when space is there */
> -            rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> -            if ( rc )
> -                ret = rc;
> +                argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
> +                /* requeue to issue a notification when space is there */
> +                rc = pending_requeue(dst_d, ring_info, src_id.domain_id, 
> len);
> +                if ( rc )
> +                    ret = rc;
> +            }
>          }
>
>          spin_unlock(&ring_info->L3_lock);
> --
> 2.17.1
>

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