Date:   Tue, 22 Feb 2022 18:09:34 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Yishai Hadas <yishaih@...dia.com>, bhelgaas@...gle.com,
        saeedm@...dia.com, linux-pci@...r.kernel.org, kvm@...r.kernel.org,
        netdev@...r.kernel.org, kuba@...nel.org, leonro@...dia.com,
        kwankhede@...dia.com, mgurtovoy@...dia.com, maorg@...dia.com,
        cohuck@...hat.com, ashok.raj@...el.com, kevin.tian@...el.com,
        shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH V8 mlx5-next 09/15] vfio: Define device migration
 protocol v2

On Tue, 22 Feb 2022 20:21:36 -0400
Jason Gunthorpe <jgg@...dia.com> wrote:

> On Tue, Feb 22, 2022 at 04:53:00PM -0700, Alex Williamson wrote:
> > On Sun, 20 Feb 2022 11:57:10 +0200
> > Yishai Hadas <yishaih@...dia.com> wrote:
> >   
> > > From: Jason Gunthorpe <jgg@...dia.com>
> > > 
> > > Replace the existing region based migration protocol with an ioctl based
> > > protocol. The two protocols have the same general semantic behaviors, but
> > > the way the data is transported is changed.
> > > 
> > > This is the STOP_COPY portion of the new protocol, it defines the 5 states
> > > for basic stop and copy migration and the protocol to move the migration
> > > data in/out of the kernel.
> > > 
> > > Compared to the clarification of the v1 protocol Alex proposed:
> > > 
> > > https://lore.kernel.org/r/163909282574.728533.7460416142511440919.stgit@omen
> > > 
> > > This has a few deliberate functional differences:
> > > 
> > >  - ERROR arcs allow the device function to remain unchanged.
> > > 
> > >  - The protocol is not required to return to the original state on
> > >    transition failure. Instead userspace can execute an unwind back to
> > >    the original state, reset, or do something else without needing kernel
> > >    support. This simplifies the kernel design and should userspace choose
> > >    a policy like always reset, avoids doing useless work in the kernel
> > >    on error handling paths.
> > > 
> > >  - PRE_COPY is made optional, userspace must discover it before using it.
> > >    This reflects the fact that the majority of drivers we are aware of
> > >    right now will not implement PRE_COPY.
> > > 
> > >  - segmentation is not part of the data stream protocol, the receiver
> > >    does not have to reproduce the framing boundaries.  
> > 
> > I'm not sure how to reconcile the statement above with:
> > 
> > 	"The user must consider the migration data segments carried
> > 	 over the FD to be opaque and non-fungible. During RESUMING, the
> > 	 data segments must be written in the same order they came out
> > 	 of the saving side FD."
> > 
> > This is subtly conflicting that it's not segmented, but segments must
> > be written in order.  We'll naturally have some segmentation due to
> > buffering in kernel and userspace, but I think referring to it as a
> > stream suggests that the user can cut and join segments arbitrarily so
> > long as byte order is preserved, right?    
> 
> Yes, it is just some odd language that carried over from the v1 language
> 
> > I suspect the commit log comment is referring to the driver imposed
> > segmentation and framing relative to region offsets.  
> 
> v1 had some special behavior where qemu would carry each data_size as
> a single unit to the other side present it whole to the migration
> region. We couldn't find any use case for this, and it wasn't clear if
> this was deliberate or just a quirk of qemu's implementation.
> 
> We tossed it because doing an extra ioctl or something to learn this
> framing would hurt a zero-copy async iouring data mover scheme.

It was deliberate in the v1 because the data region might cover both
emulated and direct mapped ranges and might do so in combinations.  For
instance the driver could create a "frame" where the header lands in
emulated space to validate sequencing and setup the fault address for
mmap access.  A driver might use a windowing scheme to iterate across a
giant framebuffer, for example.
 
> > Maybe something like:
> > 
> > 	"The user must consider the migration data stream carried over
> > 	 the FD to be opaque and must preserve the byte order of the
> > 	 stream.  The user is not required to preserve buffer
> > 	 segmentation when writing the data stream during the RESUMING
> > 	 operation."  
> 
> Yes
> 
> > > + * The kernel migration driver must fully transition the device to the new state
> > > + * value before the operation returns to the user.  
> > 
> > The above statement certainly doesn't preclude asynchronous
> > availability of data on the stream FD, but it does demand that the
> > device state transition itself is synchronous and can cannot be
> > shortcut.  If the state transition itself exceeds migration SLAs, we're
> > in a pickle.  Thanks,  
> 
> Even if the commands were async, it is not easy to believe a device
> can instantaneously abort an arc when a timer hits and return to full
> operation. For instance, mlx5 can't do this.
> 
> The vCPU cannot be restarted to try to meet the SLA until a command
> going back to RUNNING returns.
> 
> If we want to have a SLA feature it feels better to pass in the
> deadline time as part of the set state ioctl and the driver can then
> internally do something appropriate and not have to figure out how to
> juggle an external abort. The driver would be expected to return fully
> completed from STOP or return back to RUNNING before the deadline.
> 
> For instance mlx5 could possibly implement this by checking the
> migration size and doing some maths before deciding if it should
> commit to its unabortable device command.
> 
> I have a feeling supporting SLA means devices are going to have to
> report latencies for various arcs and work in a more classical
> realtime deadline oriented way overall. Estimating the transfer
> latency and size is another factor too.
> 
> Overall, this SLA topic looks quite big to me, and I think a full
> solution will come with many facets. We are also quite interested in
> dirty rate limiting, for instance.

So if/when we were to support this, we might use a different SET_STATE
feature ioctl that allows the user to specify a deadline and we'd use
feature probing or a flag on the migration feature for userspace to
discover this?  I'd be ok with that, I just want to make sure we have
agreeable options to support it.  Thanks,

Alex

Powered by blists - more mailing lists