From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant Date: Mon, 22 Aug 2016 17:29:04 +0300 Message-ID: <20160822172734-mutt-send-email-mst@kernel.org> References: <1465206694-1150-1-git-send-email-lprosek@redhat.com> <20160606165015-mutt-send-email-mst@redhat.com> <6951df63-60eb-140c-4618-4b87d21b48a4@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Paolo Bonzini , KVM list To: Ladi Prosek Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46190 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755428AbcHVO3h (ORCPT ); Mon, 22 Aug 2016 10:29:37 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4DBA881F01 for ; Mon, 22 Aug 2016 14:29:06 +0000 (UTC) Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jun 08, 2016 at 02:58:04PM +0200, Ladi Prosek wrote: > On Mon, Jun 6, 2016 at 11:31 PM, Ladi Prosek wrote: > > On Mon, Jun 6, 2016 at 4:35 PM, Paolo Bonzini 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 > >>> > >>> 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