diff mbox series

[v5,8/9] PCI: Define scoped based management functions

Message ID 20231220-cxl-cper-v5-8-1bb8a4ca2c7a@intel.com
State New
Headers show
Series efi/cxl-cper: Report CPER CXL component events through trace events | expand

Commit Message

Ira Weiny Dec. 21, 2023, 12:17 a.m. UTC
Users of pci_dev_get() can benefit from a scoped based put.  Also,
locking a PCI device is often done within a single scope.

Define a pci_dev_put() free function and a PCI device lock guard.  These
will initially be used in new CXL event processing code but is defined
in a separate patch for others to pickup and use/backport easier.

Cc: Bjorn Helgaas      <bhelgaas@google.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v5:
[Jonathan: New patch]
---
 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Dan Williams Jan. 3, 2024, 10:38 p.m. UTC | #1
[ add linux-pci ]

Ira Weiny wrote:
> Users of pci_dev_get() can benefit from a scoped based put.  Also,
> locking a PCI device is often done within a single scope.
> 
> Define a pci_dev_put() free function and a PCI device lock guard.  These
> will initially be used in new CXL event processing code but is defined
> in a separate patch for others to pickup and use/backport easier.

Hi Bjorn,

Any heartburn if I take this through cxl.git with the rest in this
series? Patch 9 has a dependency on this one.

> 
> Cc: Bjorn Helgaas      <bhelgaas@google.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes for v5:
> [Jonathan: New patch]
> ---
>  include/linux/pci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60ca768bc867..290d0a2651b2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
>  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
>  struct pci_dev *pci_dev_get(struct pci_dev *dev);
>  void pci_dev_put(struct pci_dev *dev);
> +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
>  void pci_remove_bus(struct pci_bus *b);
>  void pci_stop_and_remove_bus_device(struct pci_dev *dev);
>  void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
> @@ -1871,6 +1872,7 @@ void pci_cfg_access_unlock(struct pci_dev *dev);
>  void pci_dev_lock(struct pci_dev *dev);
>  int pci_dev_trylock(struct pci_dev *dev);
>  void pci_dev_unlock(struct pci_dev *dev);
> +DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
>  
>  /*
>   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
> 
> -- 
> 2.43.0
>
Bjorn Helgaas Jan. 3, 2024, 11:01 p.m. UTC | #2
On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > locking a PCI device is often done within a single scope.
> > 
> > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > will initially be used in new CXL event processing code but is defined
> > in a separate patch for others to pickup and use/backport easier.
> 
> Any heartburn if I take this through cxl.git with the rest in this
> series? Patch 9 has a dependency on this one.

No real heartburn.  I was trying to figure out what this does
since I'm not familiar with the "scoped based put" idea and
'git grep -i "scope.*base"' wasn't much help.

I would kind of like the commit log to say a little more about what
the "scoped based put" (does that have too many past tenses in it?)
means and how users of pci_dev_get() will benefit.

I don't know what "locking a PCI device is often done within a single
scope" is trying to tell me either.  What if it's *not* done within a
single scope?

Does this change anything for callers of pci_dev_get() and
pci_dev_put()?

Does this avoid a common programming error?  I just don't know what
the benefit of this is yet.

I'm sure this is really cool stuff, but there's little documentation,
few existing users, and I don't know what I need to look for when
reviewing things.

> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> >  void pci_dev_put(struct pci_dev *dev);
> > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> >  void pci_remove_bus(struct pci_bus *b);
> >  void pci_stop_and_remove_bus_device(struct pci_dev *dev);
> >  void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
> > @@ -1871,6 +1872,7 @@ void pci_cfg_access_unlock(struct pci_dev *dev);
> >  void pci_dev_lock(struct pci_dev *dev);
> >  int pci_dev_trylock(struct pci_dev *dev);
> >  void pci_dev_unlock(struct pci_dev *dev);
> > +DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
> >  
> >  /*
> >   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
> > 
> > -- 
> > 2.43.0
> > 
> 
>
Lukas Wunner Jan. 4, 2024, 6:05 a.m. UTC | #3
On Wed, Dec 20, 2023 at 04:17:35PM -0800, Ira Weiny wrote:
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
>  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
>  struct pci_dev *pci_dev_get(struct pci_dev *dev);
>  void pci_dev_put(struct pci_dev *dev);
> +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))

pci_dev_put() already performs a NULL pointer check internally.
Why duplicate it here?

Thanks,

Lukas
Dan Williams Jan. 4, 2024, 6:43 a.m. UTC | #4
Lukas Wunner wrote:
> On Wed, Dec 20, 2023 at 04:17:35PM -0800, Ira Weiny wrote:
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> >  void pci_dev_put(struct pci_dev *dev);
> > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> 
> pci_dev_put() already performs a NULL pointer check internally.
> Why duplicate it here?

Greg asked the same for the introduction of __free(kvfree), and Peter
clarified:

http://lore.kernel.org/r/20230814161731.GN776869@hirez.programming.kicks-ass.net

Essentially, that check is more for build-time than runtime because when
the macro is expanded the compiler can notice scenarios where @pdev is
set to NULL (likely by no_free_ptr()) and skip the call to pci_dev_put()
altogether. pci_dev_put() also happens to be out-of-line, so saving a
call when @pdev is NULL a small win in that respect as well.
Lukas Wunner Jan. 4, 2024, 7:02 a.m. UTC | #5
On Wed, Jan 03, 2024 at 10:43:40PM -0800, Dan Williams wrote:
> Lukas Wunner wrote:
> > On Wed, Dec 20, 2023 at 04:17:35PM -0800, Ira Weiny wrote:
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> > >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> > >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> > >  void pci_dev_put(struct pci_dev *dev);
> > > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> > 
> > pci_dev_put() already performs a NULL pointer check internally.
> > Why duplicate it here?
> 
> Greg asked the same for the introduction of __free(kvfree), and Peter
> clarified:
> 
> http://lore.kernel.org/r/20230814161731.GN776869@hirez.programming.kicks-ass.net
> 
> Essentially, that check is more for build-time than runtime because when
> the macro is expanded the compiler can notice scenarios where @pdev is
> set to NULL (likely by no_free_ptr()) and skip the call to pci_dev_put()
> altogether. pci_dev_put() also happens to be out-of-line, so saving a
> call when @pdev is NULL a small win in that respect as well.

Doubtful whether that's correct.  The kernel is compiled with
-fno-delete-null-pointer-checks since commit a3ca86aea507
("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").

So these NULL pointer checks are generally not optimized away.

I've just responded to the discussion you've linked above:
https://lore.kernel.org/all/20240104065744.GA6055@wunner.de/

Thanks,

Lukas
Ard Biesheuvel Jan. 4, 2024, 7:37 a.m. UTC | #6
On Thu, 4 Jan 2024 at 08:02, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Wed, Jan 03, 2024 at 10:43:40PM -0800, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > On Wed, Dec 20, 2023 at 04:17:35PM -0800, Ira Weiny wrote:
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> > > >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> > > >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> > > >  void pci_dev_put(struct pci_dev *dev);
> > > > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> > >
> > > pci_dev_put() already performs a NULL pointer check internally.
> > > Why duplicate it here?
> >
> > Greg asked the same for the introduction of __free(kvfree), and Peter
> > clarified:
> >
> > http://lore.kernel.org/r/20230814161731.GN776869@hirez.programming.kicks-ass.net
> >
> > Essentially, that check is more for build-time than runtime because when
> > the macro is expanded the compiler can notice scenarios where @pdev is
> > set to NULL (likely by no_free_ptr()) and skip the call to pci_dev_put()
> > altogether. pci_dev_put() also happens to be out-of-line, so saving a
> > call when @pdev is NULL a small win in that respect as well.
>
> Doubtful whether that's correct.  The kernel is compiled with
> -fno-delete-null-pointer-checks since commit a3ca86aea507
> ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").
>
> So these NULL pointer checks are generally not optimized away.
>
> I've just responded to the discussion you've linked above:
> https://lore.kernel.org/all/20240104065744.GA6055@wunner.de/
>

AIUI, Peter is referring to constant propagation of compile time
constant pointers here, not pointer variables where the NULL check is
elided if the variable has already been dereferenced.
Dan Williams Jan. 4, 2024, 5:41 p.m. UTC | #7
Ard Biesheuvel wrote:
> On Thu, 4 Jan 2024 at 08:02, Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Wed, Jan 03, 2024 at 10:43:40PM -0800, Dan Williams wrote:
> > > Lukas Wunner wrote:
> > > > On Wed, Dec 20, 2023 at 04:17:35PM -0800, Ira Weiny wrote:
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> > > > >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> > > > >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> > > > >  void pci_dev_put(struct pci_dev *dev);
> > > > > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> > > >
> > > > pci_dev_put() already performs a NULL pointer check internally.
> > > > Why duplicate it here?
> > >
> > > Greg asked the same for the introduction of __free(kvfree), and Peter
> > > clarified:
> > >
> > > http://lore.kernel.org/r/20230814161731.GN776869@hirez.programming.kicks-ass.net
> > >
> > > Essentially, that check is more for build-time than runtime because when
> > > the macro is expanded the compiler can notice scenarios where @pdev is
> > > set to NULL (likely by no_free_ptr()) and skip the call to pci_dev_put()
> > > altogether. pci_dev_put() also happens to be out-of-line, so saving a
> > > call when @pdev is NULL a small win in that respect as well.
> >
> > Doubtful whether that's correct.  The kernel is compiled with
> > -fno-delete-null-pointer-checks since commit a3ca86aea507
> > ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").
> >
> > So these NULL pointer checks are generally not optimized away.
> >
> > I've just responded to the discussion you've linked above:
> > https://lore.kernel.org/all/20240104065744.GA6055@wunner.de/
> >
> 
> AIUI, Peter is referring to constant propagation of compile time
> constant pointers here, not pointer variables where the NULL check is
> elided if the variable has already been dereferenced.
> 

No, it is for auto (on stack) pointer variables. Consider this sequence:

	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(...);

	if (!pdev)
		return NULL;

	if (!check_pdev(pdev))
		return NULL;

	return no_free_ptr(pdev);

...that expands at compile time to a first pass of:

	struct pci_dev *pdev = pci_get_domain_bus_and_slot(...);

	if (!pdev) {
		if (pdev)
			pci_dev_put(pdev);
		return NULL;
	}

	if (!check_pdev(pdev)) {
		if (pdev)
			pci_dev_put(pdev);
		return NULL;
	}

	struct pci_dev *tmp = pdev;
	pdev = NULL;
	if (pdev)
		pci_dev_put(pdev);
	return tmp;

...the compiler can then optimize this on a second pass to:

	if (!pdev)
		return NULL;

	if (!check_pdev(pdev)) {
		pci_dev_put(pdev);
		return NULL;
	}

	return pdev;

...if the NULL check is dropped from DEFINE_FREE(pci_dev_put...) then
this becomes unoptimizable by the compiler without
link-time-optimization (LTO) to see that pci_dev_put() has an internal
NULL check:

	struct pci_dev *pdev = pci_get_domain_bus_and_slot(...);

	if (!pdev) {
		pci_dev_put(pdev);
		return NULL;
	}

	if (!check_pdev(pdev)) {
		pci_dev_put(pdev);
		return NULL;
	}

	struct pci_dev *tmp = pdev;
	pdev = NULL;
	pci_dev_put(pdev);
	return tmp;

Now, if pci_dev_put() would become a static inline the compiler could
again do the optimization, but it is otherwise free (post compiler
optimization) to keep a conditional in these DEFINE_FREE() instances and
not worry about whether the actual free routine is inline, out-of-line,
or has its own NULL check.
Jonathan Cameron Jan. 8, 2024, 1:44 p.m. UTC | #8
On Wed, 20 Dec 2023 16:17:35 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> Users of pci_dev_get() can benefit from a scoped based put.  Also,
> locking a PCI device is often done within a single scope.
> 
> Define a pci_dev_put() free function and a PCI device lock guard.  These
> will initially be used in new CXL event processing code but is defined
> in a separate patch for others to pickup and use/backport easier.
> 
> Cc: Bjorn Helgaas      <bhelgaas@google.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

With the extra text buried deep in the discussion LGTM.
I've not been that thorough on documenting my own similar cleanup.h
series, so might well steal some of that for the ones I have outstanding
for device_handle_put() and fwnode_handle_put() ;)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for v5:
> [Jonathan: New patch]
> ---
>  include/linux/pci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60ca768bc867..290d0a2651b2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
>  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
>  struct pci_dev *pci_dev_get(struct pci_dev *dev);
>  void pci_dev_put(struct pci_dev *dev);
> +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
>  void pci_remove_bus(struct pci_bus *b);
>  void pci_stop_and_remove_bus_device(struct pci_dev *dev);
>  void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
> @@ -1871,6 +1872,7 @@ void pci_cfg_access_unlock(struct pci_dev *dev);
>  void pci_dev_lock(struct pci_dev *dev);
>  int pci_dev_trylock(struct pci_dev *dev);
>  void pci_dev_unlock(struct pci_dev *dev);
> +DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
>  
>  /*
>   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
>
diff mbox series

Patch

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60ca768bc867..290d0a2651b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1170,6 +1170,7 @@  int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
 u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
 struct pci_dev *pci_dev_get(struct pci_dev *dev);
 void pci_dev_put(struct pci_dev *dev);
+DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
 void pci_remove_bus(struct pci_bus *b);
 void pci_stop_and_remove_bus_device(struct pci_dev *dev);
 void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
@@ -1871,6 +1872,7 @@  void pci_cfg_access_unlock(struct pci_dev *dev);
 void pci_dev_lock(struct pci_dev *dev);
 int pci_dev_trylock(struct pci_dev *dev);
 void pci_dev_unlock(struct pci_dev *dev);
+DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
 
 /*
  * PCI domain support.  Sometimes called PCI segment (eg by ACPI),