Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 08, 2016 at 02:58:04PM +0200, Ladi Prosek wrote:
> On Mon, Jun 6, 2016 at 11:31 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> > On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>
> >> On 06/06/2016 15:55, Michael S. Tsirkin wrote:
> >>> > According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> >>> > negotiated the driver MUST set flags to 0. Not dirtying the available
> >>> > ring in virtqueue_disable_cb may also have a positive performance impact.
> >>>
> >>> Question would be, is this a gain or a loss. We have an extra branch,
> >>> and the write might serve to prefetch the cache line.
> >>>
> >>> > Writes to the used event field (vring_used_event) are still unconditional.
> >>> >
> >>> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >>>
> >>> Wow you are right wrt the spec. Should we change the spec or the
> >>> code though?
> >>
> >> I would change the spec and note that bit 0 of the flags is ignored if
> >> event indices are in use.
> >
> > Changing the spec sounds good. I'll see if I can get any meaningful
> > perf numbers with vring_bench, just in case. Would there be any
> > interest in having the tool checked in the tree? There are several
> > commits referencing vring_bench but it seems to exist only in a list
> > archive - took me a while to figure it out.
> 
> vring_bench with two threads, host thread polls the queue and moves
> indices from available to used, guest thread polls returned indices
> with:
> 
> do {
>   virtqueue_disable_cb(vq);
>   while ((p = virtqueue_get_buf(vq, &len)) != NULL)
>     returned++;
>   if (unlikely(virtqueue_is_broken(vq)))
>     break;
> } while (!virtqueue_enable_cb(vq));
> 
> Results:
> - no effect on branch misses
> - L1 dcache load misses ~0.5% down but with ~0.5% variance so not
> super convincing
> 
> Ladi

I think it makes sense to change this - the reason it
was written like this is because we did not have
a shadow, it was easier to change and check the flags directly.

Did you open an issue with virtio spec?

-- 
MST