On Thu, Mar 11, 2021 at 12:19 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > On Thu, Mar 11, 2021 at 11:37:28AM -0800, Alexander Duyck wrote: > > > > Then the flow for this could be changed where we take the VF lock and > > mark it as "stale" to prevent any driver binding and then we can > > release the VF lock. Next we would perform the PF operation telling it > > to update the VF. Then we spin on the VF waiting for the stale data > > to be updated and once that happens we can pop the indication that the > > device is "stale" freeing it for use. > > I always get leary when people propose to open code locking constructs > :\ I'm not suggesting we replace the lock. It is more about essentially revoking the VF. What we are doing is essentially rewriting the PCIe config of the VF so in my mind it makes sense to take sort of an RCU approach where the old one is readable, but not something a new driver can be bound to. > There is already an existing lock to prevent probe() it is the > device_lock() mutex on the VF. With no driver bound there is not much > issue to hold it over the HW activity. Yes. But sitting on those locks also has side effects such as preventing us from taking any other actions such as disabling SR-IOV. One concern I have is that if somebody else tries to implement this in the future and they don't have a synchronous setup, or worse yet they do but it takes a long time to process a request because they have a slow controller it would be preferable to just have us post the message to the PF and then have the thread spin and wait on the VF to be updated rather than block on the PF while sitting on two locks. > This lock is normally held around the entire probe() and remove() > function which has huge amounts of HW activity already. Yes, but usually that activity is time bound. You are usually reading values and processing them in a timely fashion. In the case of probe we even have cases where we have to defer because we don't want to hold these locks for too long. > 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.
