Messages in this thread | | | Date | Wed, 03 Oct 2012 15:02:35 -0400 | From | Don Dutile <> | Subject | Re: [PATCH 5/5] ixgbe: add driver set_max_vfs support |
| |
On 10/03/2012 02:47 PM, Alexander Duyck wrote: > On 10/03/2012 10:51 AM, Yinghai Lu wrote: >> Need ixgbe guys to close the loop to use set_max_vfs instead >> kernel parameters. >> >> Signed-off-by: Yinghai Lu<yinghai@kernel.org> >> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com> >> Cc: Jesse Brandeburg<jesse.brandeburg@intel.com> >> Cc: Greg Rose<gregory.v.rose@intel.com> >> Cc: "David S. Miller"<davem@davemloft.net> >> Cc: John Fastabend<john.r.fastabend@intel.com> >> Cc: e1000-devel@lists.sourceforge.net >> Cc: netdev@vger.kernel.org >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 + >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 44 +++++++++++++++++++----- >> 2 files changed, 37 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> index b9623e9..d39d975 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> @@ -558,6 +558,8 @@ struct ixgbe_adapter { >> u32 interrupt_event; >> u32 led_reg; >> >> + struct ixgbe_info *ixgbe_info; >> + >> #ifdef CONFIG_IXGBE_PTP >> struct ptp_clock *ptp_clock; >> struct ptp_clock_info ptp_caps; >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> index ee61819..1c097c7 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifier = { >> }; >> #endif >> >> -#ifdef CONFIG_PCI_IOV >> -static unsigned int max_vfs; >> -module_param(max_vfs, uint, 0); >> -MODULE_PARM_DESC(max_vfs, >> - "Maximum number of virtual functions to allocate per physical function - default is zero and maximum value is 63"); >> -#endif /* CONFIG_PCI_IOV */ >> - >> static unsigned int allow_unsupported_sfp; >> module_param(allow_unsupported_sfp, uint, 0); >> MODULE_PARM_DESC(allow_unsupported_sfp, >> @@ -4496,7 +4489,7 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter) >> #ifdef CONFIG_PCI_IOV >> /* assign number of SR-IOV VFs */ >> if (hw->mac.type != ixgbe_mac_82598EB) >> - adapter->num_vfs = (max_vfs> 63) ? 0 : max_vfs; >> + adapter->num_vfs = min_t(int, pdev->max_vfs, 63); >> >> #endif >> /* enable itr by default in dynamic mode */ >> @@ -7220,8 +7213,9 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev, >> >> #ifdef CONFIG_PCI_IOV >> ixgbe_enable_sriov(adapter, ii); >> - >> #endif >> + adapter->ixgbe_info = ii; >> + >> netdev->features = NETIF_F_SG | >> NETIF_F_IP_CSUM | >> NETIF_F_IPV6_CSUM | >> @@ -7683,11 +7677,43 @@ static const struct pci_error_handlers ixgbe_err_handler = { >> .resume = ixgbe_io_resume, >> }; >> >> +static void ixgbe_set_max_vfs(struct pci_dev *pdev) >> +{ >> +#ifdef CONFIG_PCI_IOV >> + struct ixgbe_adapter *adapter = pci_get_drvdata(pdev); >> + struct ixgbe_hw *hw =&adapter->hw; >> + int num_vfs = 0; >> + >> + /* assign number of SR-IOV VFs */ >> + if (hw->mac.type != ixgbe_mac_82598EB) >> + num_vfs = min_t(int, pdev->max_vfs, 63); >> + >> + /* no change */ >> + if (adapter->num_vfs == num_vfs) >> + return; >> + >> + if (!num_vfs) { >> + /* disable sriov */ >> + ixgbe_disable_sriov(adapter); >> + adapter->num_vfs = 0; >> + } else if (!adapter->num_vfs&& num_vfs) { >> + /* enable sriov */ >> + adapter->num_vfs = num_vfs; >> + ixgbe_enable_sriov(adapter, adapter->ixgbe_info); >> + } else { >> + /* increase or decrease */ >> + } >> + >> + pdev->max_vfs = adapter->num_vfs; >> +#endif >> +} >> + >> static struct pci_driver ixgbe_driver = { >> .name = ixgbe_driver_name, >> .id_table = ixgbe_pci_tbl, >> .probe = ixgbe_probe, >> .remove = __devexit_p(ixgbe_remove), >> + .set_max_vfs = ixgbe_set_max_vfs, >> #ifdef CONFIG_PM >> .suspend = ixgbe_suspend, >> .resume = ixgbe_resume, > > The ixgbe_set_max_vfs function has several issues. The two big ones are > that this function assumes it can just enable/disable SR-IOV without any > other changes being necessary which is not the case. I would recommend > looking at ixgbe_setup_tc for how to do this properly. Secondly is the > fact that this code will change the PF network device and as such > sections of the code should be called with the RTNL lock held. In > addition I believe you have to disable SR-IOV before enabling it again > with a different number of VFs. > > Below is a link to one of the early patches for igb when we were first > introducing SR-IOV, and the in-driver sysfs value had been rejected. I > figure it might be useful as it was also using sysfs to enable/disable > VFs. It however doesn't have the correct locking on changing the queues > and as such will likely throw an error if you were to implement it the > same way now: > http://lists.openwall.net/netdev/2009/04/08/34 > > Thanks, > > Alex
Alex, Thanks for patch set pointer. When I started to work on the ixgbe example use based on the RFC set I posted, I ran into the problem you outlined -- the PF uses/consumes all the queues & MSI intrs when sriov not enabled at driver load time, which required more network shutdown logic that I'm not familiar with... So, I was going to defer to Greg to work that magic. :) Greg: assume the 2 callback function interface in the RFC patch set I sent, (primarily, just the include/linux/pci.h changes), and you can make the necessary drivers mods from there. In the meantime, I'll make the changes to my original/v1 RFC to reflect the changes that GKH & Yinghai recommended/implemented for sysfs attribute creation & removal in a v2 posting. The end result is that the current module parameter setting for max_vfs should continue to work, and the sysfs interface will work when those pieces are provided.
-Don
| |