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@xxxxxxxxxx> wrote:
>
> > On 8/23/2021 6:16 PM, Alex Williamson wrote:
> > > On Sun, 22 Aug 2021 17:35:55 +0300
> > > Yishai Hadas <yishaih@xxxxxxxxxx> 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
