| Message ID | 1431057207-30775-1-git-send-email-weiyang@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. > Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> > --- > 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? > 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? > +#endif > + > pci_free_resources(dev); > put_device(&dev->dev); > } > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linux.vnet.ibm.com> >> --- >> 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 >>
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) { } 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); + } +#endif + pci_free_resources(dev); put_device(&dev->dev); }
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. Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> --- drivers/pci/iov.c | 42 +++++++++++++++++++++++------------------- drivers/pci/pci.h | 10 ++++++++++ drivers/pci/remove.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 19 deletions(-)