Re: [Qemu-devel] [PATCH v4 for-4.0 2/7] vhost-user: Support transferring inflight buffer between qemu and backend

[Yongji Xie]

On Tue, 15 Jan 2019 at 20:54, Michael S. Tsirkin <address@hidden> wrote:
>
> On Tue, Jan 15, 2019 at 02:46:42PM +0800, Yongji Xie wrote:
> > On Tue, 15 Jan 2019 at 06:25, Michael S. Tsirkin <address@hidden> wrote:
> > >
> > > On Wed, Jan 09, 2019 at 07:27:23PM +0800, address@hidden wrote:
> > > > @@ -382,6 +397,30 @@ If VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD protocol 
> > > > feature is negotiated,
> > > >  slave can send file descriptors (at most 8 descriptors in each message)
> > > >  to master via ancillary data using this fd communication channel.
> > > >
> > > > +Inflight I/O tracking
> > > > +---------------------
> > > > +
> > > > +To support slave reconnecting, slave need to track inflight I/O in a
> > > > +shared memory. VHOST_USER_GET_INFLIGHT_FD and 
> > > > VHOST_USER_SET_INFLIGHT_FD
> > > > +are used to transfer the memory between master and slave. And to 
> > > > encourage
> > > > +consistency, we provide a recommended format for this memory:
> > >
> > > I think we should make a stronger statement and actually
> > > just say what the format is. Not recommend it weakly.
> > >
> >
> > Okey, will do it.
> >
> > > > +
> > > > +offset        width    description
> > > > +0x0      0x400    region for queue0
> > > > +0x400    0x400    region for queue1
> > > > +0x800    0x400    region for queue2
> > > > +...      ...      ...
> > > > +
> > > > +For each virtqueue, we have a 1024 bytes region.
> > >
> > >
> > > Why is the size hardcoded? Why not a function of VQ size?
> > >
> >
> > Sorry, I didn't get your point. Should the region's size be fixed? Do
> > you mean we need to document a function for the region's size?
>
>
> Well you are saying 0x0 to 0x400 is for queue0.
> How do you know that's enough? And why are 0x400
> bytes necessary? After all max queue size can be very small.
>
>

OK, I think I get your point. So we need something like:

region's size = max_queue_size * 32 byte + xxx byte (if any)

Right?

>
> > >
> > > > The region's format is like:
> > > > +
> > > > +offset   width    description
> > > > +0x0      0x1      descriptor 0 is in use or not
> > > > +0x1      0x1      descriptor 1 is in use or not
> > > > +0x2      0x1      descriptor 2 is in use or not
> > > > +...      ...      ...
> > > > +
> > > > +For each descriptor, we use one byte to specify whether it's in use or 
> > > > not.
> > > > +
> > > >  Protocol features
> > > >  -----------------
> > > >
> > >
> > > I think that it's a good idea to have a version in this region.
> > > Otherwise how are you going to handle compatibility when
> > > this needs to be extended?
> > >
> >
> > I have put the version into the message's payload: VhostUserInflight. Is it 
> > OK?
> >
> > Thanks,
> > Yongji
>
> I'm not sure I like it.  So is qemu expected to maintain it? Reset it?
> Also don't you want to be able to detect that qemu has reset the buffer?
> If we have version 1 at a known offset that can serve both purposes.
> Given it only has value within the buffer why not store it there?
>

Yes, that looks better. Will update it in v5.

Thanks,
Yongji