All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Jason Wang <jasowang@redhat.com>
Cc: mst@redhat.com, virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	wexu@redhat.com, jfreimann@redhat.com
Subject: Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
Date: Wed, 16 May 2018 13:55:20 +0800	[thread overview]
Message-ID: <20180516055520.GA21116@debian> (raw)
In-Reply-To: <915efeaa-4e47-9004-a722-54b0c40ebcbb@redhat.com>

On Wed, May 16, 2018 at 01:01:04PM +0800, Jason Wang wrote:
> On 2018年04月25日 13:15, Tiwei Bie wrote:
[...]
> > @@ -1143,10 +1160,17 @@ static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
> >   	/* We optimistically turn back on interrupts, then check if there was
> >   	 * more to do. */
> > +	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > +	 * either clear the flags bit or point the event index at the next
> > +	 * entry. Always update the event index to keep code simple. */
> > +
> > +	vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> > +			vq->last_used_idx | (vq->wrap_counter << 15));
> 
> 
> Using vq->wrap_counter seems not correct, what we need is the warp counter
> for the last_used_idx not next_avail_idx.

Yes, you're right. I have fixed it in my local repo,
but haven't sent out a new version yet.

I'll try to send out a new RFC today.

> 
> And I think there's even no need to bother with event idx here, how about
> just set VRING_EVENT_F_ENABLE?

We had a similar discussion before. Michael prefers
to use VRING_EVENT_F_DESC when possible to avoid
extra interrupts if host is fast:

https://lkml.org/lkml/2018/4/16/1085
"""
I suspect this will lead to extra interrupts if host is fast.
So I think for now we should always use VRING_EVENT_F_DESC
if EVENT_IDX is negotiated.
"""

> 
> >   	if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> >   		virtio_wmb(vq->weak_barriers);
> > -		vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> > +		vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> > +						     VRING_EVENT_F_ENABLE;
> >   		vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> >   							vq->event_flags_shadow);
> >   	}
> > @@ -1172,15 +1196,34 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
> >   static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> >   {
> >   	struct vring_virtqueue *vq = to_vvq(_vq);
> > +	u16 bufs, used_idx, wrap_counter;
> >   	START_USE(vq);
> >   	/* We optimistically turn back on interrupts, then check if there was
> >   	 * more to do. */
> > +	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > +	 * either clear the flags bit or point the event index at the next
> > +	 * entry. Always update the event index to keep code simple. */
> > +
> > +	/* TODO: tune this threshold */
> > +	bufs = (u16)(vq->next_avail_idx - vq->last_used_idx) * 3 / 4;
> 
> bufs could be more than vq->num here, is this intended?

Yes, you're right. Like the above one -- I have fixed
it in my local repo, but haven't sent out a new version
yet. Thanks for spotting this!

> 
> > +
> > +	used_idx = vq->last_used_idx + bufs;
> > +	wrap_counter = vq->wrap_counter;
> > +
> > +	if (used_idx >= vq->vring_packed.num) {
> > +		used_idx -= vq->vring_packed.num;
> > +		wrap_counter ^= 1;
> 
> When used_idx is greater or equal vq->num, there's no need to flip
> warp_counter bit since it should match next_avail_idx.
> 
> And we need also care about the case when next_avail wraps but used_idx not.
> so we probaly need:
> 
> else if (vq->next_avail_idx < used_idx) {
>     wrap_counter ^= 1;
> }
> 
> I think maybe it's time to add some sample codes in the spec to avoid
> duplicating the efforts(bugs).

+1

Best regards,
Tiwei Bie

  parent reply	other threads:[~2018-05-16  5:54 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25  5:15 [RFC v3 0/5] virtio: support packed ring Tiwei Bie
2018-04-25  5:15 ` [RFC v3 1/5] virtio: add packed ring definitions Tiwei Bie
2018-04-25  5:15   ` Tiwei Bie
2018-04-25  5:15 ` [RFC v3 2/5] virtio_ring: support creating packed ring Tiwei Bie
2018-04-25  5:15   ` Tiwei Bie
2018-04-25  5:15 ` [RFC v3 3/5] virtio_ring: add packed ring support Tiwei Bie
2018-05-10  7:32   ` Jason Wang
2018-05-10  7:32   ` Jason Wang
2018-05-10  7:34     ` Jason Wang
2018-05-10  7:34     ` Jason Wang
2018-05-10  8:56       ` Tiwei Bie
2018-05-10  8:56       ` Tiwei Bie
2018-05-10  9:49         ` Jason Wang
2018-05-10 10:50           ` Tiwei Bie
2018-05-10 10:50             ` Tiwei Bie
2018-05-10  9:49         ` Jason Wang
2018-04-25  5:15 ` Tiwei Bie
2018-04-25  5:15 ` [RFC v3 4/5] virtio_ring: add event idx support in packed ring Tiwei Bie
2018-05-02  2:51   ` Jason Wang
2018-05-02  2:51     ` Jason Wang
2018-05-02  7:28     ` Tiwei Bie
2018-05-02 13:51       ` Michael S. Tsirkin
2018-05-02 15:12         ` Tiwei Bie
2018-05-02 15:12         ` Tiwei Bie
2018-05-02 15:42           ` Michael S. Tsirkin
2018-05-02 15:42           ` Michael S. Tsirkin
2018-05-03  1:11             ` Tiwei Bie
2018-05-03  1:11             ` Tiwei Bie
2018-05-03  1:44               ` Michael S. Tsirkin
2018-05-03  1:44               ` Michael S. Tsirkin
2018-05-03  2:09                 ` Tiwei Bie
2018-05-03  2:09                   ` Tiwei Bie
2018-05-03  7:25                   ` Jason Wang
2018-05-03  7:25                     ` Jason Wang
2018-05-03 13:54                     ` Tiwei Bie
2018-05-03 13:54                     ` Tiwei Bie
2018-05-08  3:05                       ` Jason Wang
2018-05-08  3:05                       ` Jason Wang
2018-05-08  5:40                         ` Jason Wang
2018-05-08  6:44                           ` Tiwei Bie
2018-05-08  7:16                             ` Jason Wang
2018-05-08  7:16                             ` Jason Wang
2018-05-08  9:16                               ` Tiwei Bie
2018-05-08  9:16                                 ` Tiwei Bie
2018-05-08  9:34                                 ` Jason Wang
2018-05-08  9:34                                   ` Jason Wang
2018-05-08  9:44                                   ` Tiwei Bie
2018-05-08  9:44                                   ` Tiwei Bie
2018-05-09  3:43                                     ` Jason Wang
2018-05-09  3:43                                     ` Jason Wang
2018-05-08  6:44                           ` Tiwei Bie
2018-05-08  5:40                         ` Jason Wang
2018-05-02 13:51       ` Michael S. Tsirkin
2018-05-02  7:28     ` Tiwei Bie
2018-05-16  5:01   ` Jason Wang
2018-05-16  5:55     ` Tiwei Bie
2018-05-16  5:55     ` Tiwei Bie [this message]
2018-05-16  5:01   ` Jason Wang
2018-04-25  5:15 ` Tiwei Bie
2018-04-25  5:15 ` [RFC v3 5/5] virtio_ring: enable " Tiwei Bie
2018-04-25  5:15   ` Tiwei Bie
2018-04-27  3:56 ` [RFC v3 0/5] virtio: support " Jason Wang
2018-04-27  4:18   ` Michael S. Tsirkin
2018-04-27  6:17     ` Jason Wang
2018-04-27  9:12       ` Tiwei Bie
2018-04-27  9:12       ` Tiwei Bie
2018-04-28  2:45         ` Jason Wang
2018-04-28  2:45           ` Jason Wang
2018-04-27  6:17     ` Jason Wang
2018-04-27  4:18   ` Michael S. Tsirkin
2018-04-27  3:56 ` Jason Wang
2018-05-02  3:49 ` Jason Wang
2018-05-02  3:49 ` 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=20180516055520.GA21116@debian \
    --to=tiwei.bie@intel.com \
    --cc=jasowang@redhat.com \
    --cc=jfreimann@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=wexu@redhat.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.