From: Rusty Russell <rusty@rustcorp.com.au>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org, pagupta@redhat.com
Subject: Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
In-Reply-To: <20150210101839.GA9505@redhat.com>
References: <1423471165-34243-1-git-send-email-jasowang@redhat.com> <1423471165-34243-2-git-send-email-jasowang@redhat.com> <878ug6y3uv.fsf@rustcorp.com.au> <20150210101839.GA9505@redhat.com>
User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-pc-linux-gnu)
Date: Wed, 11 Feb 2015 10:28:38 +1030
Message-ID: <87zj8lb9ox.fsf@rustcorp.com.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1836
Lines: 48

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
>> Jason Wang <jasowang@redhat.com> writes:
>> > We currently does:
>> >
>> > bufs = (avail->idx - last_used_idx) * 3 / 4;
>> >
>> > This is ok now since we only try to enable the delayed callbacks when
>> > the queue is about to be full. This may not work well when there is
>> > only one pending buffer in the virtqueue (this may be the case after
>> > tx interrupt was enabled). Since virtqueue_enable_cb() will return
>> > false which may cause unnecessary triggering of napis. This patch
>> > correct this by only calculate the four thirds when bufs is not one.
>> 
>> I mildly prefer to avoid the branch, by changing the calculation like
>> so:
>> 
>>         /* Set bufs >= 1, even if there's only one pending buffer */
>>         bufs = (bufs + 1) * 3 / 4;
>
> Or bus * 3/4 + 1
>
>> But it's not clear to me how much this happens.  I'm happy with the
>> patch though, as currently virtqueue_enable_cb_delayed() is the same
>> as virtqueue_enable_cb() if there's only been one buffer added.
>> 
>> Cheers,
>> Rusty.
>
> But isn't this by design?
> The documentation says:
>
>  * This re-enables callbacks but hints to the other side to delay
>  * interrupts until most of the available buffers have been processed;
>
> Clearly, this implies that when there's one buffer it must behave
> exactly the same.

Yes, my mistake.  We could hit the "never gets notified" case with this
change, and that's a much bigger problem.

So I don't think we can accept this patch...
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/