Date:   Tue, 22 Feb 2022 20:21:36 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.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, 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.

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

Thanks,
Jason

Powered by blists - more mailing lists