Re: [PATCH] pci/sriov: Add an option to probe VFs or not before enabling SR-IOV

[Alex Williamson]

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
> >  
>