Re: [PATCH V3 06/13] vfio/pci: Split the pci_driver code out of vfio_pci_core.c

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Aug 24, 2021 at 03:48:39PM -0600, Alex Williamson wrote:
> On Mon, 23 Aug 2021 18:28:49 +0300
> Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
> > On 8/23/2021 6:16 PM, Alex Williamson wrote:
> > > On Sun, 22 Aug 2021 17:35:55 +0300
> > > Yishai Hadas <yishaih@nvidia.com> wrote:  
> > >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > >> new file mode 100644
> > >> index 000000000000..15474ebadd98
> > >> +++ b/drivers/vfio/pci/vfio_pci.c  
> > > ...  
> > >> +static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
> > >> +{
> > >> +	might_sleep();
> > >> +
> > >> +	if (!enable_sriov)
> > >> +		return -ENOENT;
> > >> +
> > >> +	return vfio_pci_core_sriov_configure(pdev, nr_virtfn);
> > >> +}  
> > > As noted in previous version, why do we need the might_sleep() above
> > > when the core code below includes it and there's nothing above that
> > > might sleep before that?  Thanks,  
> > 
> > This is used to mention vfio_pci_core_sriov_configure might sleep.
> > 
> > If this is redundant, can you please remove this one line upon merge ?
> 
> I guess I'm not sure how far up we need to, or should, percolate
> might_sleep() annotations.  vfio_pci_core_sriov_configure() calls
> vfio_device_get_from_dev() which makes use of mutexes, which I think is
> the original reason for the annotation there ahead of those in the PCI
> iov code.  But is the annotation through mutex_lock() enough on its own,
> ie. should we remove all of our gratuitous annotations in the vfio part
> of the code path?  Thanks,

Generally you'd want to use might_sleep() on a path where the sleep is
conditional - particularly something where the conditions are rare.

Given that the mutex_lock is basically unconditional and the
!enable_sriov = false is rare, I'd suggest to delete the whole lot of
it on this path.

Jason