[PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Christophe JAILLET]

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(-)

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Bjorn Helgaas]

[+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
> 

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Logan Gunthorpe]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Bjorn Helgaas]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Logan Gunthorpe]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Bjorn Helgaas]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Logan Gunthorpe]

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

Re: [PATCH] PCI/P2PDMA: Save a few cycles in 'pci_alloc_p2pmem()'

[Bjorn Helgaas]

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
>