From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: pagupta@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
Subject: Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
Date: Wed, 11 Feb 2015 05:49:07 +0008 [thread overview]
Message-ID: <1423633267.4369.0@smtp.corp.redhat.com> (raw)
In-Reply-To: <20150210101839.GA9505@redhat.com>
On Tue, Feb 10, 2015 at 6:18 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> 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.
>
> So I'm not very happy - this changes the meaning of the API without
> updating the documentation.
Think hard about this. And looks like your are right. And the patch may
in fact cause more trouble e.g the only pending buffer were consumed
before the final check between used idx and last_used_idx.
Will drop this.
Thanks
next prev parent reply other threads:[~2015-02-11 5:41 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-09 8:39 [PATCH RFC v5 net-next 0/6] enable tx interrupts for virtio-net Jason Wang
2015-02-09 8:39 ` [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending Jason Wang
2015-02-10 1:03 ` Rusty Russell
2015-02-10 6:26 ` Jason Wang
2015-02-10 10:18 ` Michael S. Tsirkin
2015-02-10 23:58 ` Rusty Russell
2015-02-11 5:41 ` Jason Wang [this message]
2015-02-09 8:39 ` [PATCH RFC v5 net-next 2/6] virtio_ring: try to disable event index callbacks in virtqueue_disable_cb() Jason Wang
2015-02-10 1:07 ` Rusty Russell
2015-02-10 10:24 ` Michael S. Tsirkin
2015-02-11 5:55 ` Jason Wang
2015-02-09 8:39 ` [PATCH RFC v5 net-next 3/6] virtio_net: enable tx interrupt Jason Wang
2015-02-09 8:39 ` [PATCH RFC v5 net-next 4/6] virtio-net: add basic interrupt coalescing support Jason Wang
2015-02-10 1:32 ` Rusty Russell
2015-02-10 6:51 ` Jason Wang
2015-02-10 10:25 ` Michael S. Tsirkin
2015-02-10 10:40 ` Michael S. Tsirkin
2015-02-13 2:52 ` Rusty Russell
2015-02-13 12:41 ` Pawel Moll
2015-02-16 3:07 ` Rusty Russell
2015-02-13 18:19 ` Cornelia Huck
2015-02-16 3:19 ` Rusty Russell
2015-02-09 8:39 ` [PATCH RFC v5 net-next 5/6] vhost: let vhost_signal() returns whether signalled Jason Wang
2015-02-09 8:39 ` [PATCH RFC v5 net-next 6/6] vhost_net: interrupt coalescing support Jason Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1423633267.4369.0@smtp.corp.redhat.com \
--to=jasowang@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pagupta@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).