Re: [PATCH net-next V2 6/8] vhost: packed ring support

[Maxime Coquelin]

On 10/17/2018 08:54 AM, Jason Wang wrote:
> On 2018/10/16 äå9:58, Maxime Coquelin wrote:
> > On 10/15/2018 04:22 AM, Jason Wang wrote:
> > > On 2018å10æ13æ 01:23, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 12, 2018 at 10:32:44PM +0800, Tiwei Bie wrote:
> > > > > On Mon, Jul 16, 2018 at 11:28:09AM +0800, Jason Wang wrote:
> > > > > [...]
> > > > > > @@ -1367,10 +1397,48 @@ long vhost_vring_ioctl(struct vhost_dev 
> > > > > > *d, unsigned int ioctl, void __user *arg
> > > > > > ÂÂÂÂÂÂÂÂÂ vq->last_avail_idx = s.num;
> > > > > > ÂÂÂÂÂÂÂÂÂ /* Forget the cached index value. */
> > > > > > ÂÂÂÂÂÂÂÂÂ vq->avail_idx = vq->last_avail_idx;
> > > > > > +ÂÂÂÂÂÂÂ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ vq->last_avail_wrap_counter = wrap_counter;
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ vq->avail_wrap_counter = vq->last_avail_wrap_counter;
> > > > > > +ÂÂÂÂÂÂÂ }
> > > > > > ÂÂÂÂÂÂÂÂÂ break;
> > > > > > ÂÂÂÂÂ case VHOST_GET_VRING_BASE:
> > > > > > ÂÂÂÂÂÂÂÂÂ s.index = idx;
> > > > > > ÂÂÂÂÂÂÂÂÂ s.num = vq->last_avail_idx;
> > > > > > +ÂÂÂÂÂÂÂ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ s.num |= vq->last_avail_wrap_counter << 31;
> > > > > > +ÂÂÂÂÂÂÂ if (copy_to_user(argp, &s, sizeof(s)))
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ r = -EFAULT;
> > > > > > +ÂÂÂÂÂÂÂ break;
> > > > > > +ÂÂÂ case VHOST_SET_VRING_USED_BASE:
> > > > > > +ÂÂÂÂÂÂÂ /* Moving base with an active backend?
> > > > > > +ÂÂÂÂÂÂÂÂ * You don't want to do that.
> > > > > > +ÂÂÂÂÂÂÂÂ */
> > > > > > +ÂÂÂÂÂÂÂ if (vq->private_data) {
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ r = -EBUSY;
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > > > > > +ÂÂÂÂÂÂÂ }
> > > > > > +ÂÂÂÂÂÂÂ if (copy_from_user(&s, argp, sizeof(s))) {
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ r = -EFAULT;
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > > > > > +ÂÂÂÂÂÂÂ }
> > > > > > +ÂÂÂÂÂÂÂ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ wrap_counter = s.num >> 31;
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ s.num &= ~(1 << 31);
> > > > > > +ÂÂÂÂÂÂÂ }
> > > > > > +ÂÂÂÂÂÂÂ if (s.num > 0xffff) {
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ r = -EINVAL;
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > > > > > +ÂÂÂÂÂÂÂ }
> > > > > Do we want to put wrap_counter at bit 15?
> > > > I think I second that - seems to be consistent with
> > > > e.g. event suppression structure and the proposed
> > > > extension to driver notifications.
> > >
> > > Ok, I assumes packed virtqueue support 64K but looks not. I can 
> > > change it to bit 15 and GET_VRING_BASE need to be changed as well.
> > >
> > > > > If put wrap_counter at bit 31, the check (s.num > 0xffff)
> > > > > won't be able to catch the illegal index 0x8000~0xffff for
> > > > > packed ring.
> > >
> > > Do we need to clarify this in the spec?
> > >
> > > > > > +ÂÂÂÂÂÂÂ vq->last_used_idx = s.num;
> > > > > > +ÂÂÂÂÂÂÂ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ vq->last_used_wrap_counter = wrap_counter;
> > > > > > +ÂÂÂÂÂÂÂ break;
> > > > > > +ÂÂÂ case VHOST_GET_VRING_USED_BASE:
> > > > > Do we need the new VHOST_GET_VRING_USED_BASE and
> > > > > VHOST_SET_VRING_USED_BASE ops?
> > > > >
> > > > > We are going to merge below series in DPDK:
> > > > >
> > > > > http://patches.dpdk.org/patch/45874/
> > > > >
> > > > > We may need to reach an agreement first.
> > >
> > > If we agree that 64K virtqueue won't be supported, I'm ok with either.
> >
> > I'm fine to put wrap_counter at bit 15.
> > I will post a new version of the DPDK series soon.
> >
> > > Btw the code assumes used_wrap_counter is equal to avail_wrap_counter 
> > > which looks wrong?
> >
> > For split ring, we used to set the last_used_idx to the same value as
> > last_avail_idx as VHOST_USER_GET_VRING_BASE cannot be called while the
> > ring is being processed, so their value is always the same at the time
> > the request is handled.
>
>
> I may miss something, but it looks to me we should sync last_used_idx 
> from used_idx.

Ok, so as proposed off-list by Jason, we could extend
VHOST_USER_[GET|SET]_VRING_BASE to have the following payload when
VIRTIO_F_RING_PACKED is negotiated:

Bit[0:14] avail index
Bit[15] avail wrap counter
Bit[16:30] used index
Bit[31] used wrap counter

Is everyone ok with that?

Another thing that I'd like to discuss is how do we reconnect in case of
user backend crash. When it happens, the frontend hasn't queried the
backend for last_avail_idx/last_used_idx and their wrap counters.

With split ring, when it happens, we set last_avail_idx to device's used
index (see virtio_queue_restore_last_avail_idx()).
Problem with packed ring is that wrap counters information is only in 
the backend.

Can we get device's used index and deduce the wrap counter value from
corresponding descriptor flag?

Any thoughts?

Regards,
Maxime
> Thanks
>
>
> > I kept the same behavior for packed ring, and so the wrap counter have
> > to be the same.
> >
> > Regards,
> > Maxime
> >
> > > Thanks
> > >
> > > > > > +ÂÂÂÂÂÂÂ s.index = idx;
> > > > > > +ÂÂÂÂÂÂÂ s.num = vq->last_used_idx;
> > > > > > +ÂÂÂÂÂÂÂ if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ s.num |= vq->last_used_wrap_counter << 31;
> > > > > > ÂÂÂÂÂÂÂÂÂ if (copy_to_user(argp, &s, sizeof s))
> > > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂ r = -EFAULT;
> > > > > > ÂÂÂÂÂÂÂÂÂ break;
> > > > > [...]