Re: [PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2014-07-12 at 00:06 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午11:25，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> >> On Fri, 2014-07-11 at 23:19 +0800, Ethan Zhao wrote:
> >> 
> >>>> 在 2014年7月11日，下午10:54，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>>> 
> >>>> On Fri, 2014-07-11 at 22:37 +0800, Ethan Zhao wrote:
> >>>> 
> >>>>>> 在 2014年7月11日，下午10:15，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>>>>> 
> >>>>>>> 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 (david.vrabel@citrix.com)
> >>>>>>> 
> >>>>>>> v2: reorder the patchset and make it bisectable and atomic, steps clear
> >>>>>>> between interface defination and implemenation according to the
> >>>>>>> suggestion from alex.williamson@redhat.com
> >>>>>>> 
> >>>>>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> >>>>>>> ---
> >>>>>> 
> >>>>>> - 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.

> >>>>>>> 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
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired