On Thu, Mar 11, 2021 at 10:32 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > On Thu, Mar 11, 2021 at 06:53:16PM -0800, Alexander Duyck wrote: > > On Thu, Mar 11, 2021 at 3:21 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > > > > > On Thu, Mar 11, 2021 at 01:49:24PM -0800, Alexander Duyck wrote: > > > > > We don't need to invent new locks and new complexity for something > > > > > that is trivially solved already. > > > > > > > > I am not wanting a new lock. What I am wanting is a way to mark the VF > > > > as being stale/offline while we are performing the update. With that > > > > we would be able to apply similar logic to any changes in the future. > > > > > > I think we should hold off doing this until someone comes up with HW > > > that needs it. The response time here is microseconds, it is not worth > > > any complexity > > <...> > > > Another way to think of this is that we are essentially pulling a > > device back after we have already allocated the VFs and we are > > reconfiguring it before pushing it back out for usage. Having a flag > > that we could set on the VF device to say it is "under > > construction"/modification/"not ready for use" would be quite useful I > > would think. > > It is not simple flag change, but change of PCI state machine, which is > far more complex than holding two locks or call to sysfs_create_file in > the loop that made Bjorn nervous. > > I want to remind again that the suggestion here has nothing to do with > the real use case of SR-IOV capable devices in the Linux. > > The flow is: > 1. Disable SR-IOV driver autoprobe > 2. Create as much as possible VFs > 3. Wait for request from the user to get VM > 4. Change MSI-X table according to requested in item #3 > 5. Bind ready to go VF to VM > 6. Inform user about VM readiness > > The destroy flow includes VM destroy and unbind. > > Let's focus on solutions for real problems instead of trying to solve theoretical > cases that are not going to be tested and deployed. > > Thanks So part of the problem with this all along has been that you are only focused on how you are going to use this and don't think about how somebody else might need to use or implement it. In addition there are a number of half measures even within your own flow. In reality if we are thinking we are going to have to reconfigure every device it might make sense to simply block the driver from being able to load until you have configured it. Then the SR-IOV autoprobe would be redundant since you could use something like the "offline" flag to avoid that. If you are okay with step 1 where you are setting a flag to prevent driver auto probing why is it so much more overhead to set a bit blocking drivers from loading entirely while you are changing the config space? Sitting on two locks and assuming a synchronous operation is assuming a lot about the hardware and how this is going to be used. In addition it seems like the logic is that step 4 will always succeed. What happens if for example you send the message to the firmware and you don't get a response? Do you just say the request failed let the VF be used anyway? This is another reason why I would be much more comfortable with the option to offline the device and then tinker with it rather than hope that your operation can somehow do everything in one shot. In my mind step 4 really should be 4 steps. 1. Offline VF to reserve it for modification 2. Submit request for modification 3. Verify modification has occurred, reset if needed. 4. Online VF Doing it in that order allows for handling many more scenarios including those where perhaps step 2 actually consists of several changes to support any future extensions that are needed. Splitting step 2 and 3 allows for an asynchronous event where you can wait if firmware takes an excessively long time, or if step 2 somehow fails you can then repeat or revert it to get back to a consistent state. Lastly by splitting out the onlining step you can avoid potentially releasing a broken VF to be reserved if there is some sort of unrecoverable error between steps 2 and 3.
