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

[Nicholas Tsirakis <niko.tsirakis@xxxxxxxxx>]

On Tue, Jun 11, 2019 at 2:49 PM Christopher Clark
<christopher.w.clark@xxxxxxxxx> wrote:
>
> 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)
>            ~~~~~~~~~~~~~~^~~~~

Shoot, sorry about that, it compiles on my end just fine.

> 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

This was an alternative that I had considered. Ultimately I went with my current
implementation as it had less of a SLOC change, though I see now that that was
a poor choice. Shall I submit as a v2 or reply to this thread
directly? Being that
the first patch was already pushed.

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