On Tue, May 19, 2015 at 06:00:20PM -0500, Bjorn Helgaas wrote:
>On Fri, May 08, 2015 at 11:53:27AM +0800, Wei Yang wrote:
>> Beside pci_dev resources, VF has other resources between its PF, like
>> refcount to PF, sysfs link between them and the virtual bus. When a VF is
>> released in virtfn_remove(), those resources are released properly. But
>> in the hotplug case, they are not.
>>
>> In hotplug case, VFs are removed by pci_stop_and_remove_bus_device()
>> instead of virtfn_remove(). This leads to some leak for resources.
>>
>> This patch moves the resource release to pci_destroy_dev() to make sure
>> those resources are released when VF is destroyed.
>
>Earlier you mentioned a related commit, but you didn't reference it here.
>I thought you said this fixed a regression. If so, we need information
>about it. I know you said you'd like this patch to go to the stable
>kernel. We need justification for that, too, e.g., a way to reproduce a
>problem and what the effect of the problem is. I guess "hot-remove of
>an SR-IOV device" is probably the way to reproduce it, but I don't know
>what the effect is. Does a subsequent hot-add fail? Do we silently leak
>some memory? If you could open a bugzilla with a dmesg log showing the
>problem, that would be great.
>
I don't find an easy way to do "hot-remove" of an SR-IOV device. What I can
think is to export the "remove" sysfs for a VF, then see the effect. While
this needs to change the kernel to see the effect. Not sure this is
acceptable.
The effect of the "hot-remove" is, (I guess, not tested yet)
1. after VF removed, PF still has a link to VF in sysfs
2. PF is not release cleanly when all VF and PF itself is removed
Let me try to gather those information, if I could create this.
>> Signed-off-by: Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/pci/iov.c | 42 +++++++++++++++++++++++-------------------
>> drivers/pci/pci.h | 10 ++++++++++
>> drivers/pci/remove.c | 31 +++++++++++++++++++++++++++++++
>> 3 files changed, 64 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index ee0ebff..47daf2f 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -17,8 +17,6 @@
>> #include <linux/pci-ats.h>
>> #include "pci.h"
>>
>> -#define VIRTFN_ID_LEN 16
>> -
>> int pci_iov_virtfn_bus(struct pci_dev *dev, int vf_id)
>> {
>> if (!dev->is_physfn)
>> @@ -94,7 +92,7 @@ static struct pci_bus *virtfn_add_bus(struct pci_bus *bus, int busnr)
>> return child;
>> }
>>
>> -static void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
>> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus)
>> {
>> if (physbus != virtbus && list_empty(&virtbus->devices))
>> pci_remove_bus(virtbus);
>> @@ -185,9 +183,7 @@ failed:
>>
>> static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>> {
>> - char buf[VIRTFN_ID_LEN];
>> struct pci_dev *virtfn;
>> - struct pci_sriov *iov = dev->sriov;
>>
>> virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
>> pci_iov_virtfn_bus(dev, id),
>> @@ -200,24 +196,10 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>> __pci_reset_function(virtfn);
>> }
>>
>> - sprintf(buf, "virtfn%u", id);
>> - sysfs_remove_link(&dev->dev.kobj, buf);
>> - /*
>> - * pci_stop_dev() could have been called for this virtfn already,
>> - * so the directory for the virtfn may have been removed before.
>> - * Double check to avoid spurious sysfs warnings.
>> - */
>> - if (virtfn->dev.kobj.sd)
>> - sysfs_remove_link(&virtfn->dev.kobj, "physfn");
>> -
>> - mutex_lock(&iov->dev->sriov->lock);
>> pci_stop_and_remove_bus_device(virtfn);
>> - virtfn_remove_bus(dev->bus, virtfn->bus);
>> - mutex_unlock(&iov->dev->sriov->lock);
>>
>> /* balance pci_get_domain_bus_and_slot() */
>> pci_dev_put(virtfn);
>> - pci_dev_put(dev);
>> }
>>
>> int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>> @@ -760,3 +742,25 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>> return dev->sriov->total_VFs;
>> }
>> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>> +
>> +void pci_sriov_lock(struct pci_dev *dev)
>> +{
>> + struct pci_sriov *iov;
>> +
>> + if (!dev->is_physfn)
>> + return;
>> +
>> + iov = dev->sriov;
>> + mutex_lock(&iov->dev->sriov->lock);
>> +}
>> +
>> +void pci_sriov_unlock(struct pci_dev *dev)
>> +{
>> + struct pci_sriov *iov;
>> +
>> + if (!dev->is_physfn)
>> + return;
>> +
>> + iov = dev->sriov;
>> + mutex_unlock(&iov->dev->sriov->lock);
>> +}
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 9bd762c2..a0323a2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -260,6 +260,12 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
>> #endif /* CONFIG_PCI_ATS */
>>
>> #ifdef CONFIG_PCI_IOV
>> +
>> +#define VIRTFN_ID_LEN 16
>> +void pci_sriov_lock(struct pci_dev *dev);
>> +void pci_sriov_unlock(struct pci_dev *dev);
>> +void virtfn_remove_bus(struct pci_bus *physbus, struct pci_bus *virtbus);
>> +
>> int pci_iov_init(struct pci_dev *dev);
>> void pci_iov_release(struct pci_dev *dev);
>> int pci_iov_resource_bar(struct pci_dev *dev, int resno);
>> @@ -268,6 +274,10 @@ void pci_restore_iov_state(struct pci_dev *dev);
>> int pci_iov_bus_range(struct pci_bus *bus);
>>
>> #else
>> +static inline void pci_sriov_lock(struct pci_dev *dev) { }
>> +static inline void pci_sriov_unlock(struct pci_dev *dev) { }
>> +static inline void virtfn_remove_bus(struct pci_bus *physbus,
>> + struct pci_bus *virtbus) { }
>
>This doesn't make sense to me. You added calls to pci_sriov_lock(), etc.,
>but they are all under #ifdef CONFIG_PCI_IOV. So why do you need stubs
>when CONFIG_PCI_IOV is not defined?
>
Hmm, you are right.
>> static inline int pci_iov_init(struct pci_dev *dev)
>> {
>> return -ENODEV;
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 8a280e9..f2a07bf 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -41,6 +41,37 @@ static void pci_destroy_dev(struct pci_dev *dev)
>> list_del(&dev->bus_list);
>> up_write(&pci_bus_sem);
>>
>> +#ifdef CONFIG_PCI_IOV
>> + if (dev->is_virtfn) {
>> + char buf[VIRTFN_ID_LEN];
>> + int id;
>> + struct pci_dev *pdev = dev->physfn;
>> +
>> + for (id = 0; id < pci_sriov_get_totalvfs(pdev); id++) {
>> + if ((dev->bus->number == pci_iov_virtfn_bus(pdev, id))
>> + && (dev->devfn == pci_iov_virtfn_devfn(pdev, id))) {
>> + sprintf(buf, "virtfn%u", id);
>> + sysfs_remove_link(&pdev->dev.kobj, buf);
>> + break;
>> + }
>> + }
>> + /*
>> + * pci_stop_dev() could have been called for this virtfn
>> + * already, so the directory for the virtfn may have been
>> + * removed before. Double check to avoid spurious sysfs
>> + * warnings.
>> + */
>> + if (dev->dev.kobj.sd)
>> + sysfs_remove_link(&dev->dev.kobj, "physfn");
>> +
>> + pci_sriov_lock(pdev);
>> + virtfn_remove_bus(pdev->bus, dev->bus);
>> + pci_sriov_unlock(pdev);
>> +
>> + pci_dev_put(dev->physfn);
>> + }
>
>All this IOV-related code looks really out of place in pci_destroy_dev().
>Isn't there some way this code can be put in drivers/pci/iov.c?
>
Currently, I didn't find a better solution.
Let me take a look again.
>> +#endif
>> +
>> pci_free_resources(dev);
>> put_device(&dev->dev);
>> }
>> --
>> 1.7.9.5
>>
--
Richard Yang
Help you, Help me
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
