| Message ID | ab80164f4d5b32f9e6240aa4863c3a147ff9c89f.1635974126.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()' | expand |
Hi Christophe, Thank you for another nice patch! > Use 'percpu_ref_tryget_live_rcu()' instead of 'percpu_ref_tryget_live()' to > save a few cycles when it is known that the rcu lock is already > taken/released. If possible, we should explain why are we using this new API, especially since percpu_ref_tryget_live_rcu() is a relatively new addition [1], so it's obvious that its users already manage the RCU lock accordingly, and that there is no need to hold the RCU read lock again (which can in turn lead to performance improvement), which is what the percpu_ref_tryget_live() does internally. What do you think? 1. 3b13c168186c ("percpu_ref: percpu_ref_tryget_live() version holding RCU") > if (!ret) > goto out; > > - if (unlikely(!percpu_ref_tryget_live(ref))) { > + if (unlikely(!percpu_ref_tryget_live_rcu(ref))) { > gen_pool_free(p2pdma->pool, (unsigned long) ret, size); > ret = NULL; > goto out; Thank you! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
[+cc Logan, Eric] On Wed, Nov 03, 2021 at 10:16:53PM +0100, Christophe JAILLET wrote: > Use 'percpu_ref_tryget_live_rcu()' instead of 'percpu_ref_tryget_live()' to > save a few cycles when it is known that the rcu lock is already > taken/released. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Added Logan and Eric since Logan is the author and de facto maintainer of this file and Eric recently converted this to RCU. Maybe we need a MAINTAINERS entry for P2PDMA? > --- > drivers/pci/p2pdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 8d47cb7218d1..081c391690d4 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -710,7 +710,7 @@ void *pci_alloc_p2pmem(struct pci_dev *pdev, size_t size) > if (!ret) > goto out; > > - if (unlikely(!percpu_ref_tryget_live(ref))) { > + if (unlikely(!percpu_ref_tryget_live_rcu(ref))) { > gen_pool_free(p2pdma->pool, (unsigned long) ret, size); > ret = NULL; > goto out; > -- > 2.30.2 >
On 2021-12-15 10:35 a.m., Bjorn Helgaas wrote: > [+cc Logan, Eric] > > On Wed, Nov 03, 2021 at 10:16:53PM +0100, Christophe JAILLET wrote: >> Use 'percpu_ref_tryget_live_rcu()' instead of 'percpu_ref_tryget_live()' to >> save a few cycles when it is known that the rcu lock is already >> taken/released. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > Added Logan and Eric since Logan is the author and de facto maintainer > of this file and Eric recently converted this to RCU. Looks fine to me: Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > Maybe we need a MAINTAINERS entry for P2PDMA? I'm not opposed to this. Would it be a duplicate of the PCI SUBSYSTEM just with my name added as maintainer? I could send a patch if so. Logan
On Wed, Dec 15, 2021 at 02:37:51PM -0700, Logan Gunthorpe wrote: > On 2021-12-15 10:35 a.m., Bjorn Helgaas wrote: > > Maybe we need a MAINTAINERS entry for P2PDMA? > > I'm not opposed to this. Would it be a duplicate of the PCI SUBSYSTEM > just with my name added as maintainer? I could send a patch if so. Maybe something like this? Are there other relevant files? I just want to make sure that you see updates to p2pdma stuff. diff --git a/MAINTAINERS b/MAINTAINERS index 7a2345ce8521..3180160fcc28 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14717,6 +14717,20 @@ L: linux-pci@vger.kernel.org S: Supported F: Documentation/PCI/pci-error-recovery.rst +PCI PEER-TO-PEER DMA (P2PDMA) +M: Bjorn Helgaas <bhelgaas@google.com> +M: Logan Gunthorpe <logang@deltatee.com> +L: linux-pci@vger.kernel.org +S: Supported +Q: https://patchwork.kernel.org/project/linux-pci/list/ +B: https://bugzilla.kernel.org +C: irc://irc.oftc.net/linux-pci +T: git git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git +F: Documentation/PCI/ +F: Documentation/devicetree/bindings/pci/ +F: drivers/pci/p2pdma.c +F: include/linux/pci-p2pdma.h + PCI MSI DRIVER FOR ALTERA MSI IP M: Joyce Ooi <joyce.ooi@intel.com> L: linux-pci@vger.kernel.org
On 2021-12-15 2:47 p.m., Bjorn Helgaas wrote: > On Wed, Dec 15, 2021 at 02:37:51PM -0700, Logan Gunthorpe wrote: >> On 2021-12-15 10:35 a.m., Bjorn Helgaas wrote: >>> Maybe we need a MAINTAINERS entry for P2PDMA? >> >> I'm not opposed to this. Would it be a duplicate of the PCI SUBSYSTEM >> just with my name added as maintainer? I could send a patch if so. > > Maybe something like this? Are there other relevant files? I just > want to make sure that you see updates to p2pdma stuff. Largely looks good to me. The one missing file is: Documentation/driver-api/pci/p2pdma.rst Do you want me to put that in a patch or will you handle it? Thanks, Logan
On Wed, Nov 03, 2021 at 10:16:53PM +0100, Christophe JAILLET wrote: > Use 'percpu_ref_tryget_live_rcu()' instead of 'percpu_ref_tryget_live()' to > save a few cycles when it is known that the rcu lock is already > taken/released. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Applied to pci/p2pdma for v5.17, thanks! PCI/P2PDMA: Use percpu_ref_tryget_live_rcu() inside RCU critical section Since pci_alloc_p2pmem() has already called rcu_read_lock(), we're in an RCU read-side critical section and don't need to take the lock again. Use percpu_ref_tryget_live_rcu() instead of percpu_ref_tryget_live() to save a few cycles. > --- > drivers/pci/p2pdma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 8d47cb7218d1..081c391690d4 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -710,7 +710,7 @@ void *pci_alloc_p2pmem(struct pci_dev *pdev, size_t size) > if (!ret) > goto out; > > - if (unlikely(!percpu_ref_tryget_live(ref))) { > + if (unlikely(!percpu_ref_tryget_live_rcu(ref))) { > gen_pool_free(p2pdma->pool, (unsigned long) ret, size); > ret = NULL; > goto out; > -- > 2.30.2 >
On Wed, Dec 15, 2021 at 02:51:51PM -0700, Logan Gunthorpe wrote: > > > On 2021-12-15 2:47 p.m., Bjorn Helgaas wrote: > > On Wed, Dec 15, 2021 at 02:37:51PM -0700, Logan Gunthorpe wrote: > >> On 2021-12-15 10:35 a.m., Bjorn Helgaas wrote: > >>> Maybe we need a MAINTAINERS entry for P2PDMA? > >> > >> I'm not opposed to this. Would it be a duplicate of the PCI SUBSYSTEM > >> just with my name added as maintainer? I could send a patch if so. > > > > Maybe something like this? Are there other relevant files? I just > > want to make sure that you see updates to p2pdma stuff. > > Largely looks good to me. > > The one missing file is: > > Documentation/driver-api/pci/p2pdma.rst > > Do you want me to put that in a patch or will you handle it? I put the patch below on pci/p2pdma for v5.17, let me know if you want any other tweaks. I had mistakenly included these, which don't include any P2PDMA content, so I dropped them so you don't get inundated with other random changes: +F: Documentation/PCI/ +F: Documentation/devicetree/bindings/pci/ commit bdebed96bd4d ("MAINTAINERS: Add Logan Gunthorpe as P2PDMA maintainer") Author: Bjorn Helgaas <bhelgaas@google.com> Date: Wed Dec 15 15:43:04 2021 -0600 MAINTAINERS: Add Logan Gunthorpe as P2PDMA maintainer Add a P2PDMA entry to make sure Logan is aware of changes to that area. Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/MAINTAINERS b/MAINTAINERS index 7a2345ce8521..ea59e32e1e81 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14717,6 +14717,19 @@ L: linux-pci@vger.kernel.org S: Supported F: Documentation/PCI/pci-error-recovery.rst +PCI PEER-TO-PEER DMA (P2PDMA) +M: Bjorn Helgaas <bhelgaas@google.com> +M: Logan Gunthorpe <logang@deltatee.com> +L: linux-pci@vger.kernel.org +S: Supported +Q: https://patchwork.kernel.org/project/linux-pci/list/ +B: https://bugzilla.kernel.org +C: irc://irc.oftc.net/linux-pci +T: git git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git +F: Documentation/driver-api/pci/p2pdma.rst +F: drivers/pci/p2pdma.c +F: include/linux/pci-p2pdma.h + PCI MSI DRIVER FOR ALTERA MSI IP M: Joyce Ooi <joyce.ooi@intel.com> L: linux-pci@vger.kernel.org
On 2021-12-15 3:04 p.m., Bjorn Helgaas wrote: > On Wed, Dec 15, 2021 at 02:51:51PM -0700, Logan Gunthorpe wrote: >> Largely looks good to me. >> >> The one missing file is: >> >> Documentation/driver-api/pci/p2pdma.rst >> >> Do you want me to put that in a patch or will you handle it? > > I put the patch below on pci/p2pdma for v5.17, let me know if you want > any other tweaks. > > I had mistakenly included these, which don't include any P2PDMA > content, so I dropped them so you don't get inundated with other > random changes: > > +F: Documentation/PCI/ > +F: Documentation/devicetree/bindings/pci/ Yup, ok. Looks good to me. If you want or need my Acked-by or whatever, you can add it: Acked-by: Logan Gunthorpe <logang@deltatee.com> Thanks, Logan
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 8d47cb7218d1..081c391690d4 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -710,7 +710,7 @@ void *pci_alloc_p2pmem(struct pci_dev *pdev, size_t size) if (!ret) goto out; - if (unlikely(!percpu_ref_tryget_live(ref))) { + if (unlikely(!percpu_ref_tryget_live_rcu(ref))) { gen_pool_free(p2pdma->pool, (unsigned long) ret, size); ret = NULL; goto out;
Use 'percpu_ref_tryget_live_rcu()' instead of 'percpu_ref_tryget_live()' to save a few cycles when it is known that the rcu lock is already taken/released. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/pci/p2pdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)