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

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

On Tue, Jun 11, 2019 at 10:11 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
> 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.

You're correct that this is an issue to be fixed, but unfortunately
this patch doesn't compile, at least with gcc 8.2 with warnings as
errors, reporting:

argo.c: In function 'sendv':
argo.c:2057:35: error: passing argument 3 of 'iov_count' from
incompatible pointer type [-Werror=incompatible-pointer-types]
             iov_count(iovs, niov, &len);
                                   ^~~~
argo.c:723:25: note: expected 'unsigned int *' but argument is of type
'long unsigned int *'
           unsigned int *count)
           ~~~~~~~~~~~~~~^~~~~

Even without this error, the logic it implements can unnecessarily
invoke iov_count twice upon the same guest-supplied buffers; it would
be better to avoid that, so: looking at the original section of code:

* sendv's "len" variable can be int, rather than long.
* iov_count can be called from sendv, just before ringbuf_insert,
instead of within ringbuf_insert. It can populate sendv's "len"
variable.
* the len obtained from iov_count (if successful) can be passed into
ringbuf_insert as a parameter, and replace ringbuf_insert's existing
"len" variable.
* ringbuf_insert's "out_len" pointer argument can then be dropped as
unnecessary.
* pending_requeue will be fine to use sendv's populated "len" variable.

Christopher


> Signed-off-by: Nicholas Tsirakis <tsirakisn@xxxxxxxxxxxx>
> ---
>  xen/common/argo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 2f874a570d..eb541829d6 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -2050,6 +2050,12 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>          {
>              int rc;
>
> +            /*
> +             * if ringbuf_insert fails, then len will never be populated.
> +             * make sure to populate it here.
> +             */
> +            iov_count(iovs, niov, &len);
> +
>              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);
> --
> 2.17.1
>

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