> 在 2014年7月11日，下午11:25，Alex Williamson <[email protected]> 写道： > >> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote: >> >>>> 在 2014年7月11日，下午10:54，Alex Williamson <[email protected]> 写道： >>>> >>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote: >>>> >>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <[email protected]> 写道： >>>>>> >>>>>>> On Fri, 2014-07-11 at 07:13 -0600, Alex Williamson wrote: >>>>>>> On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote: >>>>>>> This patch introduces two new device assignment functions >>>>>>> >>>>>>> pci_iov_assign_device(), >>>>>>> pci_iov_deassign_device() >>>>>>> >>>>>>> along with the existed one >>>>>>> >>>>>>> pci_vfs_assigned() >>>>>>> >>>>>>> They construct the VFs assignment management interface, used to assign/ >>>>>>> deassign device to VM and query the VFs reference counter. instead of >>>>>>> direct manipulation of device flag. >>>>>>> >>>>>>> This patch refashioned the related code and make them atomic. >>>>>>> >>>>>>> v3: change the naming of device assignment helpers, because they work >>>>>>> for all kind of PCI device, not only SR-IOV ([email protected]) >>>>>>> >>>>>>> v2: reorder the patchset and make it bisectable and atomic, steps clear >>>>>>> between interface defination and implemenation according to the >>>>>>> suggestion from [email protected] >>>>>>> >>>>>>> Signed-off-by: Ethan Zhao <[email protected]> >>>>>>> --- >>>>>> >>>>>> - Use a cover patch to describe the series >>>>>> >>>>>> - Version information goes here, below the "---", not above it >>>>>> >>>>>> - I stand by the patch breakdown I suggested originally, there are too >>>>>> many logical changes here in patch 1. There are easily 3 separate >>>>>> patches here. >>>>>> >>>>>> - Renaming s/sriov/iov/ doesn't address the problem David raised. The >>>>>> name is still synonymous with SR-IOV and it's defined on >>>>>> drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV. >>>>>> >>>>>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it >>>>>> not removed? >>>>> >>>>> I guess it's still used, which is even worse because now we have >>>>> separate data elements, one tracking how many VFs are assigned from a PF >>>>> and another tracking each device that's assigned. What are we actually >>>>> gaining or fixing by doing this? >>>> >>>> Of course it is used to tracking PF/VF assigned, what we gained are - we >>>> hide the details >>>> In assignment helpers, we wouldn't bother the users anymore and implement >>>> what inside >>>> The helpers, my initial purpose is to simplify the ugly >>>> pci_vfs_assigned() with counter. >>>> >>>> I don't want to push more change in one step, that would out of control to >>>> get perfect result. >>> >>> But a counter means that we're tracking the data in two separate places, >>> which is generally a bad idea. Cleaning up direct access to flags is >>> fine, but creating a separate counter doesn't really seem to add any >>> value. >> >> If you are asked how many apples in your bag , you like to count them one by >> one and >> Give the answer ? What are you doing when you put them into the bag ? > > If performance mattered, then yes, keeping a count may be advantageous. > Performance does not matter in any path where this is used. > Not only performance, concurrency side-effect it caused. And less complication, lesser buggy. >>>>>>> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 17 >>>>>>> ++--------------- >>>>>>> drivers/pci/iov.c | 20 >>>>>>> ++++++++++++++++++++ >>>>>>> drivers/xen/xen-pciback/pci_stub.c | 4 ++-- >>>>>>> include/linux/pci.h | 4 ++++ >>>>>>> virt/kvm/assigned-dev.c | 2 +- >>>>>>> virt/kvm/iommu.c | 4 ++-- >>>>>> >>>>>> - This patch touch 4 separate logical code areas, who do you expect to >>>>>> ack and commit it? Split it up. >>>>>> >>>>>>> 6 files changed, 31 insertions(+), 20 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >>>>>>> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >>>>>>> index 02c11a7..781040e 100644 >>>>>>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >>>>>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c >>>>>>> @@ -693,22 +693,9 @@ complete_reset: >>>>>>> static bool i40e_vfs_are_assigned(struct i40e_pf *pf) >>>>>>> { >>>>>>> struct pci_dev *pdev = pf->pdev; >>>>>>> - struct pci_dev *vfdev; >>>>>>> - >>>>>>> - /* loop through all the VFs to see if we own any that are assigned >>>>>>> */ >>>>>>> - vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL); >>>>>>> - while (vfdev) { >>>>>>> - /* if we don't own it we don't care */ >>>>>>> - if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) { >>>>>>> - /* if it is assigned we cannot release it */ >>>>>>> - if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED) >>>>>>> - return true; >>>>>>> - } >>>>>>> >>>>>>> - vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, >>>>>>> - I40E_DEV_ID_VF, >>>>>>> - vfdev); >>>>>>> - } >>>>>>> + if (pci_vfs_assigned(pdev)) >>>>>>> + return true; >>>>>>> >>>>>>> return false; >>>>>>> } >>>>>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >>>>>>> index de7a747..090f827 100644 >>>>>>> --- a/drivers/pci/iov.c >>>>>>> +++ b/drivers/pci/iov.c >>>>>>> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev) >>>>>>> EXPORT_SYMBOL_GPL(pci_vfs_assigned); >>>>>>> >>>>>>> /** >>>>>>> + * pci_iov_assign_device - assign device to VM >>>>>>> + * @pdev: the device to be assigned. >>>>>>> + */ >>>>>>> +void pci_iov_assign_device(struct pci_dev *pdev) >>>>>>> +{ >>>>>>> + pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_assign_device); >>>>>>> + >>>>>>> +/** >>>>>>> + * pci_iov_deassign_device - deasign device from VM >>>>>>> + * @pdev: the device to be deassigned. >>>>>>> + */ >>>>>>> +void pci_iov_deassign_device(struct pci_dev *pdev) >>>>>>> +{ >>>>>>> + pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device); >>>>>>> + >>>>>>> +/** >>>>>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available >>>>>>> * @dev: the PCI PF device >>>>>>> * @numvfs: number that should be used for TotalVFs supported >>>>>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c >>>>>>> b/drivers/xen/xen-pciback/pci_stub.c >>>>>>> index 62fcd48..27e00d1 100644 >>>>>>> --- a/drivers/xen/xen-pciback/pci_stub.c >>>>>>> +++ b/drivers/xen/xen-pciback/pci_stub.c >>>>>>> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref >>>>>>> *kref) >>>>>>> xen_pcibk_config_free_dyn_fields(dev); >>>>>>> xen_pcibk_config_free_dev(dev); >>>>>>> >>>>>>> - dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >>>>>>> + pci_iov_deassign_device(dev); >>>>>>> pci_dev_put(dev); >>>>>>> >>>>>>> kfree(psdev); >>>>>>> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev) >>>>>>> dev_dbg(&dev->dev, "reset device\n"); >>>>>>> xen_pcibk_reset_device(dev); >>>>>>> >>>>>>> - dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; >>>>>>> + pci_iov_assign_device(dev); >>>>>>> return 0; >>>>>>> >>>>>>> config_release: >>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>>>>> index aab57b4..5ece6d6 100644 >>>>>>> --- a/include/linux/pci.h >>>>>>> +++ b/include/linux/pci.h >>>>>>> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev); >>>>>>> int pci_vfs_assigned(struct pci_dev *dev); >>>>>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); >>>>>>> int pci_sriov_get_totalvfs(struct pci_dev *dev); >>>>>>> +void pci_iov_assign_device(struct pci_dev *dev); >>>>>>> +void pci_iov_deassign_device(struct pci_dev *dev); >>>>>>> #else >>>>>>> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn) >>>>>>> { return -ENODEV; } >>>>>>> @@ -1614,6 +1616,8 @@ static inline int pci_sriov_set_totalvfs(struct >>>>>>> pci_dev *dev, u16 numvfs) >>>>>>> { return 0; } >>>>>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) >>>>>>> { return 0; } >>>>>>> +static inline void pci_iov_assign_device(struct pci_dev *dev) { } >>>>>>> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { } >>>>>>> #endif >>>>>>> >>>>>>> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE) >>>>>>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c >>>>>>> index bf06577..7fac58d 100644 >>>>>>> --- a/virt/kvm/assigned-dev.c >>>>>>> +++ b/virt/kvm/assigned-dev.c >>>>>>> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm >>>>>>> *kvm, >>>>>>> else >>>>>>> pci_restore_state(assigned_dev->dev); >>>>>>> >>>>>>> - assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >>>>>>> + pci_iov_deassign_device(assigned_dev->dev); >>>>>>> >>>>>>> pci_release_regions(assigned_dev->dev); >>>>>>> pci_disable_device(assigned_dev->dev); >>>>>>> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c >>>>>>> index 0df7d4b..641ee73 100644 >>>>>>> --- a/virt/kvm/iommu.c >>>>>>> +++ b/virt/kvm/iommu.c >>>>>>> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm, >>>>>>> goto out_unmap; >>>>>>> } >>>>>>> >>>>>>> - pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; >>>>>>> + pci_iov_assign_device(pdev); >>>>>>> >>>>>>> dev_info(&pdev->dev, "kvm assign device\n"); >>>>>>> >>>>>>> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm, >>>>>>> >>>>>>> iommu_detach_device(domain, &pdev->dev); >>>>>>> >>>>>>> - pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED; >>>>>>> + pci_iov_deassign_device(pdev); >>>>>>> >>>>>>> dev_info(&pdev->dev, "kvm deassign device\n"); >>> >>> >>> > > >
------------------------------------------------------------------------------ _______________________________________________ E1000-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
