On Mon, 20 Mar 2017 18:34:23 -0500
Bodong Wang <bodong@xxxxxxxxxxxx> wrote:
> On 3/20/2017 6:07 PM, Gavin Shan wrote:
> > On Mon, Mar 20, 2017 at 05:14:34PM +0200, bodong@xxxxxxxxxxxx wrote:
> >> From: Bodong Wang <bodong@xxxxxxxxxxxx>
> >>
> >> Sometimes it is not desirable to probe the virtual functions after
> >> SRIOV is enabled. This can save host side resource usage by VF
> >> instances which would be eventually probed to VMs.
What resources would not be released when the VF driver is unbound?
> >> Added a new PCI sysfs interface "sriov_probe_vfs" to control that
> > >from PF, all current callers still retain the same functionality.
> >> To modify it, echo 0/n/N (disable probe) or 1/y/Y (enable probe) to
> >>
> >> /sys/bus/pci/devices/<DOMAIN:BUS:DEVICE.FUNCTION>/sriov_probe_vfs
> >>
> >> Note that, the choice must be made before enabling VFs. The change
> >> will not take effect if VFs are already enabled. Simply, one can set
> >> sriov_numvfs to 0, choose whether to probe or not, and then resume
> >> sriov_numvfs.
> >>
> > Bodong, I'm not sure if there is a requirement to load driver for the
> > specified number of VFs? That indicates no driver will be loaded for
> > other VFs. If so, this interface might serve the purpose as well.
> Gavin, thanks for the review. That is indeed an interesting suggestion.
> Theoretically, we can change that probe_vfs from boolean to integer.
> And use it as a counter to probe the first N VFs(if N < total_vfs).
> Let's see if there are any objections.
Is it just me or does this seem like a confusing user interface, ie. to
get binary on/off behavior a user now needs to 'cat total_vfs >
sriov_probe_vfs'. It's not very intuitive, what's the use case for it?
> >
> >
> > +#ifdef CONFIG_PCI_IOV
> > + if (!pci_dev->is_virtfn ||
> > + (pci_dev->is_virtfn && pci_dev->physfn->sriov->probe_vfs)) {
> > +#endif
> > + error = __pci_device_probe(drv, pci_dev);
> > + if (error) {
> > + pcibios_free_irq(pci_dev);
> > + pci_dev_put(pci_dev);
> > + }
> > +#ifdef CONFIG_PCI_IOV
> > }
> > +#endif
> >
> > I think it's reasonable to have a inline function for this check:
> It's doable, but what's the benefit?
Way cleaner.
> >
> > #ifdef CONFIG_PCI_IOV
> > static inline bool pci_device_can_probe(struct pci_dev *pdev)
> > {
> > return (!pdev->is_virtfn || pdev->physfn->sriov->probe_vfs);
> should be return (!pdev->is_virtfn || (pci_dev->is_virtfn &&
> pci_dev->physfn->sriov->probe_vfs));
>
> We want to probe that device if 1) it's a PF 2) it'a VF and probe_vfs is set
> > }
> > #else
> > static inline bool pci_device_can_probe(struct pci_dev *pdev)
> > {
> > return true;
> > }
> This function will be a waste if CONFIG_PCI_IOV is not defined.
> > #endif
>
> > Thanks,
> > Gavin
> >
>
