| 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 |
[ 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 >
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 > > > >
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
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.
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
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.
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.
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 --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),
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(+)