Re: [Qemu-devel] [PATCH v5 1/6] vhost-user: Support transferring inflight buffer between qemu and backend

[Yongji Xie]

On Tue, 29 Jan 2019 at 12:26, Michael S. Tsirkin <address@hidden> wrote:
>
> On Tue, Jan 29, 2019 at 12:11:55PM +0800, Stefan Hajnoczi wrote:
> > On Tue, Jan 22, 2019 at 04:31:47PM +0800, address@hidden wrote:
> > > +typedef struct DescState {
> > > +    uint8_t inuse;
> > > +    uint8_t version;
> > > +    uint16_t used_idx;
> > > +    uint16_t avail_idx;
> > > +    uint16_t reserved;
> > > +} DescState;
> > > +
> > > +typedef struct QueueRegion {
> > > +    uint8_t valid;
>
> what's this?
>

We can use this to check whether this buffer is reset by qemu.

> > > +    uint16_t desc_num;
>
> there's padding before this field. Pls make it explicit.
>

Will do it.

> > > +    DescState desc[0];
> > > +} QueueRegion;
> > > +
> > > +The struct DescState is used to describe one head-descriptor's state. The
> > > +fields have following meanings:
> > > +
> > > +    inuse: Indicate whether the descriptor is inuse or not.
>
> inuse by what?
>

Maybe inflight is better?

> > > +
> > > +    version: Indicate whether we have an atomic update to used ring and
> > > +    inflight buffer when slave crash at that point. This field should be
> > > +    increased by one before and after this two updates. An odd version
> > > +    indicates an in-progress update.
>
> I'm not sure I understand what does the above say. Also does this
> require two atomics? Seems pretty expensive. And why is it called
> version?
>
> > > +
> > > +    used_idx: Store old index of used ring before we update used ring and
> > > +    inflight buffer so that slave can know whether an odd version 
> > > inflight
> > > +    head-descriptor in inflight buffer is processed or not.
>
> Here too.
>

Sorry, the above description may be not clear. This two fields are
used to indicate whether we have an in-progress update to used ring
and inflight buffer. If slave crash before the update to used_ring and
after the update to inflight buffer, the version should be odd and
used_idx should be equal to used_ring.idx. Then we need to roll back
the update to inflight buffer. As for the name of the version filed,
actually I didn't find a good one, so I just copy it from struct
kvm_steal_time...

> > > +
> > > +    avail_idx: Used to preserve the descriptor's order in avail ring so 
> > > that
> > > +    slave can resubmit descriptors in order.
>
> Why would that be necessary?
>

Maybe some devices will be able to use it to preserve order after
reconnecting in future?

> >
> > Will a completely new "packed vring" inflight shm layout be necessary to
> > support the packed vring layout in VIRTIO 1.1?
> >
> > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-610007
>
> Probably.
>

How about supporting packed virtqueue in guest driver?

Thanks,
Yongji