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

[Michael S. Tsirkin]

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?

> > +    uint16_t desc_num;

there's padding before this field. Pls make it explicit.

> > +    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?

> > +
> > +    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.

> > +
> > +    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?

> 
> 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.

-- 
MST