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

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

On Tue, Jun 11, 2019 at 12:55 PM Nicholas Tsirakis
<niko.tsirakis@xxxxxxxxx> wrote:
>
> 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.
>
> 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.

v2 please, ensuring that it applies to staging on top of the prior
patch already applied.

thanks

Christopher

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