* [PATCH] pci/iov: return a reference to PF on destroying VF @ 2015-04-10 8:53 Wei Yang 2015-05-05 21:29 ` Bjorn Helgaas 2015-05-08 3:53 ` [PATCH V2] pci/iov: fix resource leak " Wei Yang 0 siblings, 2 replies; 11+ messages in thread From: Wei Yang @ 2015-04-10 8:53 UTC (permalink / raw) To: bhelgaas, gwshan; +Cc: linux-pci, Wei Yang Each VF will get a reference to its PF, while it is not returned back in all cases and leave a removed PF's pci_dev un-released. As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates, when removing devices on a bus, we do it in the reverse order. This means we would remove VFs first, then PFs. After doing so, VF's removal is done with pci_stop_and_remove_bus_device() instead of virtfn_remove(). virtfn_remove() returns the reference of its PF, while pci_stop_and_remove_bus_device() doesn't. This patches moves the return of PF's reference to pci_destroy_dev() to make sure the PF's pci_dev is released in any case. Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> --- drivers/pci/iov.c | 1 - drivers/pci/remove.c | 5 +++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 4b3a4ea..9b04bde 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset) /* balance pci_get_domain_bus_and_slot() */ pci_dev_put(virtfn); - pci_dev_put(dev); } static int sriov_enable(struct pci_dev *dev, int nr_virtfn) diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 8bd76c9..836ddf6 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -41,6 +41,11 @@ 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) + pci_dev_put(dev->physfn); +#endif + pci_free_resources(dev); put_device(&dev->dev); } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-04-10 8:53 [PATCH] pci/iov: return a reference to PF on destroying VF Wei Yang @ 2015-05-05 21:29 ` Bjorn Helgaas 2015-05-06 6:09 ` Wei Yang 2015-05-06 7:25 ` Wei Yang 2015-05-08 3:53 ` [PATCH V2] pci/iov: fix resource leak " Wei Yang 1 sibling, 2 replies; 11+ messages in thread From: Bjorn Helgaas @ 2015-05-05 21:29 UTC (permalink / raw) To: Wei Yang; +Cc: gwshan, linux-pci On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote: > Each VF will get a reference to its PF, while it is not returned back in > all cases and leave a removed PF's pci_dev un-released. > > As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates, > when removing devices on a bus, we do it in the reverse order. This means > we would remove VFs first, then PFs. After doing so, VF's removal is done > with pci_stop_and_remove_bus_device() instead of virtfn_remove(). > virtfn_remove() returns the reference of its PF, while > pci_stop_and_remove_bus_device() doesn't. Please use conventional citation style (12-char SHA1). ac205b7bb72f appeared in v3.4. Did that commit cause a regression? Should this patch be marked for stable? "After doing so, VF removal is done with pci_stop_and_remove_device() ..." After doing what? After removing the VFs and PFs? After commit ac205b7bb72f? Prior to your patch, the VF reference was released in virtfn_remove(), which is only called via pci_disable_sriov(). This typically happens in a driver .remove() method. The reference is *not* released if we call pci_stop_and_remove_bus_device(VF) directly, as we would via the remove_store() (sysfs "remove" file) or hot unplug paths, e.g., pciehp_unconfigure_device(). After your patch, the VF reference is released in pci_destroy_dev(). This is called from pci_disable_sriov(), so it happens in that path as before. But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so the reference is now released for all the paths that use pci_stop_and_remove_bus_device(). What about the other things done in virtfn_remove(), e.g., the sysfs link removal? Your patch fixes a reference count leak, but don't we still have a sysfs link leak? It would be useful to mention a way to cause the leak. I suspect writing to a VF's sysfs "remove" file is the easiest. > This patches moves the return of PF's reference to pci_destroy_dev() to > make sure the PF's pci_dev is released in any case. > > Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> > --- > drivers/pci/iov.c | 1 - > drivers/pci/remove.c | 5 +++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 4b3a4ea..9b04bde 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset) > > /* balance pci_get_domain_bus_and_slot() */ > pci_dev_put(virtfn); > - pci_dev_put(dev); > } > > static int sriov_enable(struct pci_dev *dev, int nr_virtfn) > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > index 8bd76c9..836ddf6 100644 > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -41,6 +41,11 @@ 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) > + pci_dev_put(dev->physfn); > +#endif > + > pci_free_resources(dev); > put_device(&dev->dev); > } > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-05 21:29 ` Bjorn Helgaas @ 2015-05-06 6:09 ` Wei Yang 2015-05-06 15:30 ` Bjorn Helgaas 2015-05-06 7:25 ` Wei Yang 1 sibling, 1 reply; 11+ messages in thread From: Wei Yang @ 2015-05-06 6:09 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Wei Yang, gwshan, linux-pci On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: >On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote: >> Each VF will get a reference to its PF, while it is not returned back in >> all cases and leave a removed PF's pci_dev un-released. >> >> As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates, >> when removing devices on a bus, we do it in the reverse order. This means >> we would remove VFs first, then PFs. After doing so, VF's removal is done >> with pci_stop_and_remove_bus_device() instead of virtfn_remove(). >> virtfn_remove() returns the reference of its PF, while >> pci_stop_and_remove_bus_device() doesn't. Hi, Bjorn Nice to see you again :-) > >Please use conventional citation style (12-char SHA1). sure, I'd like to change it. > >ac205b7bb72f appeared in v3.4. Did that commit cause a regression? >Should this patch be marked for stable? > Hmm... regression. I think commit ac205b7bb72f is not a complete fix for the problem. Before commit ac205b7bb72f, system would crash, and after that, at least we can use the machine. While yes, I prefer this could be in stable tree. >"After doing so, VF removal is done with pci_stop_and_remove_device() ..." >After doing what? After removing the VFs and PFs? After commit >ac205b7bb72f? Sorry for my poor expression. Here I mean, after commit ac205b7bb72f. Before this commit ac205b7bb72f, VFs are destroyed in sriov_disable() called by the PF's driver. After this commit ac205b7bb72f, since we reverse the order, VFs are destroyed by the pci_stop_and_remove_bus_device() in the loop. > >Prior to your patch, the VF reference was released in virtfn_remove(), >which is only called via pci_disable_sriov(). This typically happens in >a driver .remove() method. The reference is *not* released if we call >pci_stop_and_remove_bus_device(VF) directly, as we would via the >remove_store() (sysfs "remove" file) or hot unplug paths, e.g., >pciehp_unconfigure_device(). You want to say the reference for VF or PF? VF's reference is still released in virtfn_remove. pci_stop_and_remove_bus_device() will call pci_destroy_dev() which will put the dev's reference. Maybe I don't get your point. > >After your patch, the VF reference is released in pci_destroy_dev(). This >is called from pci_disable_sriov(), so it happens in that path as before. >But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so >the reference is now released for all the paths that use >pci_stop_and_remove_bus_device(). > The change in this patch is the reference release of the PF. Before my patch, PF's reference is released in virtfn_remove(). After my patch, PF's reference is released in the pci_destroy_dev() of the VF. >What about the other things done in virtfn_remove(), e.g., the sysfs link >removal? Your patch fixes a reference count leak, but don't we still have >a sysfs link leak? > Agree, I am afraid the sysfs would have a leak too. While I am not that familiar with the sysfs part, I don't dare to move that to pci_destroy_dev(). Need more investigation. >It would be useful to mention a way to cause the leak. I suspect writing >to a VF's sysfs "remove" file is the easiest. > I use the EEH hotplug case to see the leak, sounds your way is more general. I will do some tests, and if it is true, I will put it in the change log. >> This patches moves the return of PF's reference to pci_destroy_dev() to >> make sure the PF's pci_dev is released in any case. >> >> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >> --- >> drivers/pci/iov.c | 1 - >> drivers/pci/remove.c | 5 +++++ >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >> index 4b3a4ea..9b04bde 100644 >> --- a/drivers/pci/iov.c >> +++ b/drivers/pci/iov.c >> @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset) >> >> /* balance pci_get_domain_bus_and_slot() */ >> pci_dev_put(virtfn); >> - pci_dev_put(dev); >> } >> >> static int sriov_enable(struct pci_dev *dev, int nr_virtfn) >> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c >> index 8bd76c9..836ddf6 100644 >> --- a/drivers/pci/remove.c >> +++ b/drivers/pci/remove.c >> @@ -41,6 +41,11 @@ 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) >> + pci_dev_put(dev->physfn); >> +#endif >> + >> pci_free_resources(dev); >> put_device(&dev->dev); >> } >> -- >> 1.7.9.5 >> -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-06 6:09 ` Wei Yang @ 2015-05-06 15:30 ` Bjorn Helgaas 2015-05-07 2:35 ` Wei Yang 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2015-05-06 15:30 UTC (permalink / raw) To: Wei Yang; +Cc: Gavin Shan, linux-pci On Wed, May 6, 2015 at 1:09 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: > On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: >>Prior to your patch, the VF reference was released in virtfn_remove(), >>which is only called via pci_disable_sriov(). This typically happens in >>a driver .remove() method. The reference is *not* released if we call >>pci_stop_and_remove_bus_device(VF) directly, as we would via the >>remove_store() (sysfs "remove" file) or hot unplug paths, e.g., >>pciehp_unconfigure_device(). > > You want to say the reference for VF or PF? Yes, I meant the PF reference. The hot unplug paths call pci_stop_and_remove_bus_device() for the VFs first, then the PF. Calling it for the VF releases the VF reference but not the PF one. Calling it for the PF will call virtfn_remove() via the driver's .remove() method, but it probably does nothing because when it calls pci_get_domain_bus_and_slot() to get the virtfn, it gets a NULL because the VF has already been removed. >>What about the other things done in virtfn_remove(), e.g., the sysfs link >>removal? Your patch fixes a reference count leak, but don't we still have >>a sysfs link leak? > > Agree, I am afraid the sysfs would have a leak too. > > While I am not that familiar with the sysfs part, I don't dare to move that to > pci_destroy_dev(). Need more investigation. OK. I don't want to fix half of the leak problem. I want to fix the whole thing at once. Bjorn ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-06 15:30 ` Bjorn Helgaas @ 2015-05-07 2:35 ` Wei Yang 0 siblings, 0 replies; 11+ messages in thread From: Wei Yang @ 2015-05-07 2:35 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Wei Yang, Gavin Shan, linux-pci On Wed, May 06, 2015 at 10:30:39AM -0500, Bjorn Helgaas wrote: >On Wed, May 6, 2015 at 1:09 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: >> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: > >>>Prior to your patch, the VF reference was released in virtfn_remove(), >>>which is only called via pci_disable_sriov(). This typically happens in >>>a driver .remove() method. The reference is *not* released if we call >>>pci_stop_and_remove_bus_device(VF) directly, as we would via the >>>remove_store() (sysfs "remove" file) or hot unplug paths, e.g., >>>pciehp_unconfigure_device(). >> >> You want to say the reference for VF or PF? > >Yes, I meant the PF reference. > >The hot unplug paths call pci_stop_and_remove_bus_device() for the VFs >first, then the PF. Calling it for the VF releases the VF reference >but not the PF one. Calling it for the PF will call virtfn_remove() >via the driver's .remove() method, but it probably does nothing >because when it calls pci_get_domain_bus_and_slot() to get the virtfn, >it gets a NULL because the VF has already been removed. > >>>What about the other things done in virtfn_remove(), e.g., the sysfs link >>>removal? Your patch fixes a reference count leak, but don't we still have >>>a sysfs link leak? >> >> Agree, I am afraid the sysfs would have a leak too. >> >> While I am not that familiar with the sysfs part, I don't dare to move that to >> pci_destroy_dev(). Need more investigation. > >OK. I don't want to fix half of the leak problem. I want to fix the >whole thing at once. > Sure, I will do some investigation and repost it. BTW, seems we also face the leak of the virtual bus. Will fix it in next version too. >Bjorn -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-05 21:29 ` Bjorn Helgaas 2015-05-06 6:09 ` Wei Yang @ 2015-05-06 7:25 ` Wei Yang 2015-05-06 15:23 ` Bjorn Helgaas 1 sibling, 1 reply; 11+ messages in thread From: Wei Yang @ 2015-05-06 7:25 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Wei Yang, gwshan, linux-pci On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: >On Fri, Apr 10, 2015 at 04:53:04PM +0800, Wei Yang wrote: >> Each VF will get a reference to its PF, while it is not returned back in >> all cases and leave a removed PF's pci_dev un-released. >> >> As commit ac205b7b ("PCI: make sriov work with hotplug remove") indicates, >> when removing devices on a bus, we do it in the reverse order. This means >> we would remove VFs first, then PFs. After doing so, VF's removal is done >> with pci_stop_and_remove_bus_device() instead of virtfn_remove(). >> virtfn_remove() returns the reference of its PF, while >> pci_stop_and_remove_bus_device() doesn't. > >Please use conventional citation style (12-char SHA1). > >ac205b7bb72f appeared in v3.4. Did that commit cause a regression? >Should this patch be marked for stable? > >"After doing so, VF removal is done with pci_stop_and_remove_device() ..." >After doing what? After removing the VFs and PFs? After commit >ac205b7bb72f? > >Prior to your patch, the VF reference was released in virtfn_remove(), >which is only called via pci_disable_sriov(). This typically happens in >a driver .remove() method. The reference is *not* released if we call >pci_stop_and_remove_bus_device(VF) directly, as we would via the >remove_store() (sysfs "remove" file) or hot unplug paths, e.g., >pciehp_unconfigure_device(). > >After your patch, the VF reference is released in pci_destroy_dev(). This >is called from pci_disable_sriov(), so it happens in that path as before. >But pci_destroy_dev() is called from pci_stop_and_remove_bus_device(), so >the reference is now released for all the paths that use >pci_stop_and_remove_bus_device(). > >What about the other things done in virtfn_remove(), e.g., the sysfs link >removal? Your patch fixes a reference count leak, but don't we still have >a sysfs link leak? > >It would be useful to mention a way to cause the leak. I suspect writing >to a VF's sysfs "remove" file is the easiest. > Looks a VF don't support the remove now. static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj, struct attribute *a, int n) { struct device *dev = container_of(kobj, struct device, kobj); struct pci_dev *pdev = to_pci_dev(dev); if (pdev->is_virtfn) return 0; return a->mode; } static struct attribute_group pci_dev_hp_attr_group = { .attrs = pci_dev_hp_attrs, .is_visible = pci_dev_hp_attrs_are_visible, }; >> This patches moves the return of PF's reference to pci_destroy_dev() to >> make sure the PF's pci_dev is released in any case. >> >> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >> --- >> drivers/pci/iov.c | 1 - >> drivers/pci/remove.c | 5 +++++ >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >> index 4b3a4ea..9b04bde 100644 >> --- a/drivers/pci/iov.c >> +++ b/drivers/pci/iov.c >> @@ -167,7 +167,6 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset) >> >> /* balance pci_get_domain_bus_and_slot() */ >> pci_dev_put(virtfn); >> - pci_dev_put(dev); >> } >> >> static int sriov_enable(struct pci_dev *dev, int nr_virtfn) >> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c >> index 8bd76c9..836ddf6 100644 >> --- a/drivers/pci/remove.c >> +++ b/drivers/pci/remove.c >> @@ -41,6 +41,11 @@ 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) >> + pci_dev_put(dev->physfn); >> +#endif >> + >> pci_free_resources(dev); >> put_device(&dev->dev); >> } >> -- >> 1.7.9.5 >> -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-06 7:25 ` Wei Yang @ 2015-05-06 15:23 ` Bjorn Helgaas 2015-05-07 2:40 ` Wei Yang 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2015-05-06 15:23 UTC (permalink / raw) To: Wei Yang; +Cc: Gavin Shan, linux-pci On Wed, May 6, 2015 at 2:25 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: > On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: >>It would be useful to mention a way to cause the leak. I suspect writing >>to a VF's sysfs "remove" file is the easiest. >> > > Looks a VF don't support the remove now. > > static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj, > struct attribute *a, int n) > { > struct device *dev = container_of(kobj, struct device, kobj); > struct pci_dev *pdev = to_pci_dev(dev); > > if (pdev->is_virtfn) > return 0; > > return a->mode; > } > > static struct attribute_group pci_dev_hp_attr_group = { > .attrs = pci_dev_hp_attrs, > .is_visible = pci_dev_hp_attrs_are_visible, > }; Right, I forgot about this. A VF has no "remove" file, so it can't be used to cause the leak. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pci/iov: return a reference to PF on destroying VF 2015-05-06 15:23 ` Bjorn Helgaas @ 2015-05-07 2:40 ` Wei Yang 0 siblings, 0 replies; 11+ messages in thread From: Wei Yang @ 2015-05-07 2:40 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Wei Yang, Gavin Shan, linux-pci On Wed, May 06, 2015 at 10:23:26AM -0500, Bjorn Helgaas wrote: >On Wed, May 6, 2015 at 2:25 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: >> On Tue, May 05, 2015 at 04:29:05PM -0500, Bjorn Helgaas wrote: > >>>It would be useful to mention a way to cause the leak. I suspect writing >>>to a VF's sysfs "remove" file is the easiest. >>> >> >> Looks a VF don't support the remove now. >> >> static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj, >> struct attribute *a, int n) >> { >> struct device *dev = container_of(kobj, struct device, kobj); >> struct pci_dev *pdev = to_pci_dev(dev); >> >> if (pdev->is_virtfn) >> return 0; >> >> return a->mode; >> } >> >> static struct attribute_group pci_dev_hp_attr_group = { >> .attrs = pci_dev_hp_attrs, >> .is_visible = pci_dev_hp_attrs_are_visible, >> }; > >Right, I forgot about this. A VF has no "remove" file, so it can't be >used to cause the leak. Hmm, I see this is introduced in commit dfab88beda88, to prevent some memory leak. Could we say, if we do the release properly like in this patch, we could re-enable this "remove"? -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V2] pci/iov: fix resource leak on destroying VF 2015-04-10 8:53 [PATCH] pci/iov: return a reference to PF on destroying VF Wei Yang 2015-05-05 21:29 ` Bjorn Helgaas @ 2015-05-08 3:53 ` Wei Yang 2015-05-19 23:00 ` Bjorn Helgaas 1 sibling, 1 reply; 11+ messages in thread From: Wei Yang @ 2015-05-08 3:53 UTC (permalink / raw) To: bhelgaas, gwshan; +Cc: linux-pci, Wei Yang 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(-) 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); } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2] pci/iov: fix resource leak on destroying VF 2015-05-08 3:53 ` [PATCH V2] pci/iov: fix resource leak " Wei Yang @ 2015-05-19 23:00 ` Bjorn Helgaas 2015-05-20 1:34 ` Wei Yang 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2015-05-19 23:00 UTC (permalink / raw) To: Wei Yang; +Cc: gwshan, linux-pci 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] pci/iov: fix resource leak on destroying VF 2015-05-19 23:00 ` Bjorn Helgaas @ 2015-05-20 1:34 ` Wei Yang 0 siblings, 0 replies; 11+ messages in thread From: Wei Yang @ 2015-05-20 1:34 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Wei Yang, gwshan, linux-pci 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 >> -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-20 1:35 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-10 8:53 [PATCH] pci/iov: return a reference to PF on destroying VF Wei Yang 2015-05-05 21:29 ` Bjorn Helgaas 2015-05-06 6:09 ` Wei Yang 2015-05-06 15:30 ` Bjorn Helgaas 2015-05-07 2:35 ` Wei Yang 2015-05-06 7:25 ` Wei Yang 2015-05-06 15:23 ` Bjorn Helgaas 2015-05-07 2:40 ` Wei Yang 2015-05-08 3:53 ` [PATCH V2] pci/iov: fix resource leak " Wei Yang 2015-05-19 23:00 ` Bjorn Helgaas 2015-05-20 1:34 ` Wei Yang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.