Date:   Thu, 3 Feb 2022 10:17:05 -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
Subject: Re: [PATCH V6 mlx5-next 08/15] vfio: Define device migration
 protocol v2

On Wed, Feb 02, 2022 at 04:36:56PM -0700, Alex Williamson wrote:

> > So you are asking to remove "is not reliable" and just phrase is as:
> > 
> > "device_state is updated to the current value when -1 is returned,
> > except when these XXX errnos are returned?
> > 
> > (actually userspace can tell directly without checking the errno - as
> > if -1 is returned the device_state cannot be the requested target
> > state anyhow)
> 
> If we decide to keep the existing code, then yes the spec should
> indicate the device_state is invalid, not just unreliable for those
> errnos, but I'm also of the opinion that returning an error condition
> AND providing valid data in the return structure for all but a few
> errnos and expecting userspace to get this correct is not a good API.

It was done this way because we didn't see any use case for the
reading the device_state except debugging, and adding another ioctl
and driver op just to get the device_state without a real user looked
like overkill.

As you already analyzed, despite the scary label in the comment, this
return is actually fully reliable so long as the userspace is
operating the API correctly - eg checking the feature flag and so on.

So, it is not as scary as you are making it out to be - and yes maybe
GET on FEATURE is cleaner.

> > It is allowed to transition to RUNNING due to reset events it captures
> > and since we capture the reset through the PCI hook, not from VFIO,
> > the core code doesn't synchronize well. See patch 14
> 
> Looking... your .reset_done() function sets a deferred_reset flag and
> attempts to grab the state_mutex.  If there's contention on that mutex,
> exit since the lock holder will perform the state transition when
> dropping that mutex, otherwise reset_done will itself drop the mutex to
> do that state change.  The reset_lock assures that we cannot race as the
> state_mutex is being released.
> 
> So the scenario is that the user MUST be performing a reset coincident
> to accessing the device_state and the solution is that the user's
> SET_STATE returns success and a new device state that's already bogus
> due to the reset.

Er, no, you suggested the core code could just cache the return since
it cannot change and then use that cached value as though it is
correct. As reset happens outside the core call chain's view it means
any core cache becomes out of sync. It is not a race, it just means
we can't cache the value in the core.

> Why wouldn't the solution here be to return -EAGAIN to the user or
> reattempt the SET_STATE since the user is clearly now disconnected
> from the actual device_state?

This is just a race that the user inflicted on themselves. We protect
kernel integrity and choose to resolve the race as though the
set_state happened first in time and the reset happened second in
time.

The API is not designed to be used concurrently, so it is a user error
if they hit this.

> > We can do this too, but it is a bunch of code to achieve this and I
> > don't have any use case to read back the device_state beyond debugging
> > and debugging is fine with this. IMHO
> 
> A bunch of code?  If we use a FEATURE ioctl, it just extends the
> existing implementation to add GET support.  That looks rather trivial.
> That seems like a selling point for using the FEATURE ioctl TBH.

We didn't even define a driver op to return the current state, trivial
code yes, but code nonetheless.

> > Things like the cap chains enter a whole world of adventure for
> > strace/syzkaller :)
> 
> vfio's argsz/flags is not only a standard framework, but it's one that
> promotes extensions.  We were able to add capability chains with
> backwards compatibility because of this design.  

IHMO the formal cap chains in the INFO ioctls were a mistake. The
argsz/flags already provide enough extension capability to return the
few extra fields directly by growing the main struct through argsz and
that handles most of what is in the caps.

The few variable size caps, like iova ranges, would have been simpler
as system calls that return only that data. This avoids userspace from
having to do all the memory allocation stuff just to read a single u32
when they don't have an interest in, say, ranges.

> initially did not see the fit for setting device state as interacting
> with a device feature, but it doesn't seem like you had a strong
> objection to my explanation of it in that context.

I don't have a strong feeling here. I think as the maintainer you
should just set a clear philosophy for ioctls in VFIO and communicate
it. There are many choices, most are reasonable.

We tried the FEATURE path, and it is OK of course, but it looks weird
as set_state is in/out due to the data_fd but it being used with
SET. I can't say that it is any better, and diffstate says it is more
code.

> > > Should that be an ioctl on the data stream FD itself?    
> > 
> > I can be. Implementation wise it is about a wash.
> > 
> > > Is there a use case for also having it on the STOP_COPY FD?  
> > 
> > I didn't think of one worthwhile enough to mandate implementing it in
> > every driver.
> 
> Can the user perform an lseek(2) on the migration FD?  Maybe that would
> be the difference between what we need for PRE_COPY vs STOP_COPY.  In
> the latter case the data should be a fixes size and perhaps we don't
> need another interface to know how much data to expect.

I'm leary to abuse the FD interface this way, we setup the FD as
noseek, like a pipe, and the core fd code has some understanding of
this.

> One use case would be that we want to be able to detect whether we can
> meet service guarantees as quickly as possible with the minimum
> resource consumption and downtime.  If we can determine from the device
> that we can't possibly transfer its state in the required time, we can
> abort immediately without waiting for a downtime exception or flooding
> the migration link.  Thanks,

It is an idea, but I don't know how to translate bytes to time, we
don't know how fast the device can generate the data for instance.

Jason 

Powered by blists - more mailing lists