> 在 2014年7月12日，上午12:13，Alex Williamson <[email protected]> 写道：
>
>> On Sat, 2014-07-12 at 00:06 +0800, Ethan Zhao wrote:
>>
>>>> 在 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.
>
> I'd argue that it doesn't fix anything there, the count can still change
> after it's been read but before any action has been taken based on the
> count. The only thing it does is eliminate the act of re-counting.
If no counter, how to avoid traversing devices and get VFs assigned count ? To
avoid
Re-counting is enough I think.
>
>>>>>>>>> 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
