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

[Michael S. Tsirkin]

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.



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

-- 
MST