| Message ID | 20231220-cxl-cper-v5-8-1bb8a4ca2c7a@intel.com |
|---|---|
| State | Accepted |
| Commit | ced085ef369af7a2b6da962ec2fbd01339f60693 |
| 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 > > > >
Bjorn Helgaas wrote: > 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. That is completely fair, and I should have checked to make sure that the changelog clarified the impact of the change. > 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. Here a is a re-write of the changelog: --- PCI: Introduce cleanup helpers for device reference counts and locks The "goto error" pattern is notorious for introducing subtle resource leaks. Use the new cleanup.h helpers for PCI device reference counts and locks. Similar to the new put_device() and device_lock() cleanup helpers, __free(put_device) and guard(device), define the same for PCI devices, __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the need for "goto free;" and "goto unlock;" patterns. For example, A 'struct pci_dev *' instance declared as: struct pci_dev *pdev __free(pci_dev_put) = NULL; ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev goes out of scope (automatic variable scope). If a function wants to invoke pci_dev_put() on error, but return @pdev on success, it can do: return no_free_ptr(pdev); ...or: return_ptr(pdev); For potential cleanup opportunity there are 587 open-coded calls to pci_dev_put() in the kernel with 65 instances within 10 lines of a goto statement with the CXL driver threatening to add another one. The guard() helper holds the associated lock for the remainder of the current scope in which it was invoked. So, for example: func(...) { if (...) { ... guard(pci_dev); /* pci_dev_lock() invoked here */ ... } /* <- implied pci_dev_unlock() triggered here */ } There are 15 invocations of pci_dev_unlock() in the kernel with 5 instances within 10 lines of a goto statement. Again, the CXL driver is threatening to add another. Introduce these helpers to preclude the addition of new more error prone goto put; / goto unlock; sequences. For now, these helpers are used in drivers/cxl/pci.c to allow ACPI error reports to be fed back into the CXL driver associated with the PCI device identified in the report. --- As for reviewing conversions that use these new helpers, one of the gotchas I have found is that it is easy to make a mistake if a goto still exists in the function after the conversion. So unless and until all of the resources a function acquires, that currently need a goto to unwind them on error, can be converted to cleanup.h based helpers it is best not to mix styles. I think the function documentation in include/linux/cleanup.h does a decent job of explaining how to use the helpers. However, I am happy to suggest some updates if you think it would help.
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.
Dan Williams wrote: > Bjorn Helgaas wrote: > > 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. > > That is completely fair, and I should have checked to make sure that the > changelog clarified the impact of the change. Agreed. Apologies for the brevity. > > > 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? I was not trying to fix that but Dan covers it below indicating that the pointer can be returned outside the scope if needed with no_free_ptr(). > > > > Does this change anything for callers of pci_dev_get() and > > pci_dev_put()? Current callers don't need to use this. > > > > Does this avoid a common programming error? I just don't know what > > the benefit of this is yet. Dan covered it well below. > > > > 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. > > Here a is a re-write of the changelog: FWIW Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > PCI: Introduce cleanup helpers for device reference counts and locks > > The "goto error" pattern is notorious for introducing subtle resource > leaks. Use the new cleanup.h helpers for PCI device reference counts and > locks. > > Similar to the new put_device() and device_lock() cleanup helpers, > __free(put_device) and guard(device), define the same for PCI devices, > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > need for "goto free;" and "goto unlock;" patterns. For example, A > 'struct pci_dev *' instance declared as: > > struct pci_dev *pdev __free(pci_dev_put) = NULL; > > ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev > goes out of scope (automatic variable scope). If a function wants to > invoke pci_dev_put() on error, but return @pdev on success, it can do: > > return no_free_ptr(pdev); > > ...or: > > return_ptr(pdev); > > For potential cleanup opportunity there are 587 open-coded calls to > pci_dev_put() in the kernel with 65 instances within 10 lines of a goto > statement with the CXL driver threatening to add another one. > > The guard() helper holds the associated lock for the remainder of the > current scope in which it was invoked. So, for example: > > func(...) > { > if (...) { > ... > guard(pci_dev); /* pci_dev_lock() invoked here */ > ... > } /* <- implied pci_dev_unlock() triggered here */ > } > > There are 15 invocations of pci_dev_unlock() in the kernel with 5 > instances within 10 lines of a goto statement. Again, the CXL driver is > threatening to add another. > > Introduce these helpers to preclude the addition of new more error prone > goto put; / goto unlock; sequences. For now, these helpers are used in > drivers/cxl/pci.c to allow ACPI error reports to be fed back into the > CXL driver associated with the PCI device identified in the report. > > --- > > As for reviewing conversions that use these new helpers, one of the > gotchas I have found is that it is easy to make a mistake if a goto > still exists in the function after the conversion. So unless and until > all of the resources a function acquires, that currently need a goto to > unwind them on error, can be converted to cleanup.h based helpers it is > best not to mix styles. > > I think the function documentation in include/linux/cleanup.h does a > decent job of explaining how to use the helpers. However, I am happy to > suggest some updates if you think it would help.
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, Jan 03, 2024 at 04:21:02PM -0800, Dan Williams wrote: > Bjorn Helgaas wrote: > > 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. > > That is completely fair, and I should have checked to make sure that the > changelog clarified the impact of the change. I see "scoped based put" follows a similar use in cleanup.h, but I think "scope-based X" reads better. > > 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. > > Here a is a re-write of the changelog: > > --- > PCI: Introduce cleanup helpers for device reference counts and locks > > The "goto error" pattern is notorious for introducing subtle resource > leaks. Use the new cleanup.h helpers for PCI device reference counts and > locks. > > Similar to the new put_device() and device_lock() cleanup helpers, > __free(put_device) and guard(device), define the same for PCI devices, > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > need for "goto free;" and "goto unlock;" patterns. For example, A > 'struct pci_dev *' instance declared as: > > struct pci_dev *pdev __free(pci_dev_put) = NULL; I see several similar __free() uses with NULL initializations in gpio, but I think this idiom would be slightly improved if the __free() function were more closely associated with the actual pci_dev_get(): struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...); Not always possible, I know, but easier to analyze when it is. > ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev > goes out of scope (automatic variable scope). If a function wants to > invoke pci_dev_put() on error, but return @pdev on success, it can do: > > return no_free_ptr(pdev); > > ...or: > > return_ptr(pdev); > > For potential cleanup opportunity there are 587 open-coded calls to > pci_dev_put() in the kernel with 65 instances within 10 lines of a goto > statement with the CXL driver threatening to add another one. > > The guard() helper holds the associated lock for the remainder of the > current scope in which it was invoked. So, for example: > > func(...) > { > if (...) { > ... > guard(pci_dev); /* pci_dev_lock() invoked here */ > ... > } /* <- implied pci_dev_unlock() triggered here */ > } Thanks for this! I had skimmed cleanup.h previously, but it makes a lot more sense after your description here. I think a little introduction along these lines would be even more useful in cleanup.h since the concept is general and not PCI-specific. E.g., the motivation (avoid resource leaks with "goto error" pattern), a definition of "__free() based cleanup function" (IIUC, a function to be run when a variable goes out of scope), maybe something about ordering (it's important in the "goto error" pattern that the cleanups are done in a specific order; how does that translate to __free()?) But the commit log above is fine with me. (It does contain tabs, which get slightly mangled when "git log" indents it.) > There are 15 invocations of pci_dev_unlock() in the kernel with 5 > instances within 10 lines of a goto statement. Again, the CXL driver is > threatening to add another. > > Introduce these helpers to preclude the addition of new more error prone > goto put; / goto unlock; sequences. For now, these helpers are used in > drivers/cxl/pci.c to allow ACPI error reports to be fed back into the > CXL driver associated with the PCI device identified in the report. This part is also fine but doesn't seem strictly necessary to me. I think the part about error reports being fed back needs a lot more background to understand the connection, and probably only makes sense in the context of that patch. > As for reviewing conversions that use these new helpers, one of the > gotchas I have found is that it is easy to make a mistake if a goto > still exists in the function after the conversion. So unless and until > all of the resources a function acquires, that currently need a goto to > unwind them on error, can be converted to cleanup.h based helpers it is > best not to mix styles. > > I think the function documentation in include/linux/cleanup.h does a > decent job of explaining how to use the helpers. However, I am happy to > suggest some updates if you think it would help. Thanks, Dan! Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Bjorn Helgaas wrote: [..] > > --- > > PCI: Introduce cleanup helpers for device reference counts and locks > > > > The "goto error" pattern is notorious for introducing subtle resource > > leaks. Use the new cleanup.h helpers for PCI device reference counts and > > locks. > > > > Similar to the new put_device() and device_lock() cleanup helpers, > > __free(put_device) and guard(device), define the same for PCI devices, > > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > > need for "goto free;" and "goto unlock;" patterns. For example, A > > 'struct pci_dev *' instance declared as: > > > > struct pci_dev *pdev __free(pci_dev_put) = NULL; > > I see several similar __free() uses with NULL initializations in gpio, > but I think this idiom would be slightly improved if the __free() > function were more closely associated with the actual pci_dev_get(): > > struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...); > > Not always possible, I know, but easier to analyze when it is. I tend to agree, but it does lead to some long lines, for example: diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 4fd1f207c84e..549ba4b8294e 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, struct cxl_cper_event_rec *rec) { struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; - struct pci_dev *pdev __free(pci_dev_put) = NULL; enum cxl_event_log_type log_type; struct cxl_dev_state *cxlds; unsigned int devfn; u32 hdr_flags; devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, - device_id->bus_num, devfn); + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( + device_id->segment_num, device_id->bus_num, devfn); if (!pdev) return; ...so I think people are choosing the "... __free(x) = NULL;" style for code density readability. > > > ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev > > goes out of scope (automatic variable scope). If a function wants to > > invoke pci_dev_put() on error, but return @pdev on success, it can do: > > > > return no_free_ptr(pdev); > > > > ...or: > > > > return_ptr(pdev); > > > > For potential cleanup opportunity there are 587 open-coded calls to > > pci_dev_put() in the kernel with 65 instances within 10 lines of a goto > > statement with the CXL driver threatening to add another one. > > > > The guard() helper holds the associated lock for the remainder of the > > current scope in which it was invoked. So, for example: > > > > func(...) > > { > > if (...) { > > ... > > guard(pci_dev); /* pci_dev_lock() invoked here */ > > ... > > } /* <- implied pci_dev_unlock() triggered here */ > > } > > Thanks for this! I had skimmed cleanup.h previously, but it makes a > lot more sense after your description here. > > I think a little introduction along these lines would be even more > useful in cleanup.h since the concept is general and not PCI-specific. Ok, let me ponder an update here. > E.g., the motivation (avoid resource leaks with "goto error" pattern), > a definition of "__free() based cleanup function" (IIUC, a function to > be run when a variable goes out of scope), maybe something about > ordering (it's important in the "goto error" pattern that the cleanups > are done in a specific order; how does that translate to __free()?) The __free() callbacks are invoked in reverse declaration (FILO) order. However, as I say that another reviewer recommendation falls out. Be careful about the variable declaration order diverging from the init order. I.e. save the reader from needing to wonder if there are intra variable init order dependencies by making it clear that init order == declaration order. > > But the commit log above is fine with me. (It does contain tabs, > which get slightly mangled when "git log" indents it.) > > > There are 15 invocations of pci_dev_unlock() in the kernel with 5 > > instances within 10 lines of a goto statement. Again, the CXL driver is > > threatening to add another. > > > > Introduce these helpers to preclude the addition of new more error prone > > goto put; / goto unlock; sequences. For now, these helpers are used in > > drivers/cxl/pci.c to allow ACPI error reports to be fed back into the > > CXL driver associated with the PCI device identified in the report. > > This part is also fine but doesn't seem strictly necessary to me. I > think the part about error reports being fed back needs a lot more > background to understand the connection, and probably only makes sense > in the context of that patch. Sure I can trim that out and just say that the CXL driver is one such occasion where a new goto for pci_dev_put() and pci_dev_unlock() was about to be introduced. > > As for reviewing conversions that use these new helpers, one of the > > gotchas I have found is that it is easy to make a mistake if a goto > > still exists in the function after the conversion. So unless and until > > all of the resources a function acquires, that currently need a goto to > > unwind them on error, can be converted to cleanup.h based helpers it is > > best not to mix styles. > > > > I think the function documentation in include/linux/cleanup.h does a > > decent job of explaining how to use the helpers. However, I am happy to > > suggest some updates if you think it would help. > > Thanks, Dan! > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks, Bjorn!
Dan Williams wrote: > Bjorn Helgaas wrote: > [..] > > > --- > > > PCI: Introduce cleanup helpers for device reference counts and locks > > > > > > The "goto error" pattern is notorious for introducing subtle resource > > > leaks. Use the new cleanup.h helpers for PCI device reference counts and > > > locks. > > > > > > Similar to the new put_device() and device_lock() cleanup helpers, > > > __free(put_device) and guard(device), define the same for PCI devices, > > > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > > > need for "goto free;" and "goto unlock;" patterns. For example, A > > > 'struct pci_dev *' instance declared as: > > > > > > struct pci_dev *pdev __free(pci_dev_put) = NULL; > > > > I see several similar __free() uses with NULL initializations in gpio, > > but I think this idiom would be slightly improved if the __free() > > function were more closely associated with the actual pci_dev_get(): > > > > struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...); > > > > Not always possible, I know, but easier to analyze when it is. > > I tend to agree, but it does lead to some long lines, for example: > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 4fd1f207c84e..549ba4b8294e 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > struct cxl_cper_event_rec *rec) > { > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > enum cxl_event_log_type log_type; > struct cxl_dev_state *cxlds; > unsigned int devfn; > u32 hdr_flags; > > devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, > - device_id->bus_num, devfn); > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > + device_id->segment_num, device_id->bus_num, devfn); > if (!pdev) > return; > > ...so I think people are choosing the "... __free(x) = NULL;" style for > code density readability. > Also in this case we need devfn assigned first. Is the above patch compliant with current style guidelines? Or would it be better to do? diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index b14237f824cf..8a180c6abb67 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, struct cxl_cper_event_rec *rec) { struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; - struct pci_dev *pdev __free(pci_dev_put) = NULL; enum cxl_event_log_type log_type; struct cxl_dev_state *cxlds; - unsigned int devfn; + unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( + device_id->segment_num, + device_id->bus_num, devfn); u32 hdr_flags; - devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, - device_id->bus_num, devfn); if (!pdev) return; Ira
On Thu, Jan 04, 2024 at 01:46:56PM -0800, Ira Weiny wrote: > Dan Williams wrote: > > Bjorn Helgaas wrote: > > [..] > > > > --- > > > > PCI: Introduce cleanup helpers for device reference counts and locks > > > > > > > > The "goto error" pattern is notorious for introducing subtle resource > > > > leaks. Use the new cleanup.h helpers for PCI device reference counts and > > > > locks. > > > > > > > > Similar to the new put_device() and device_lock() cleanup helpers, > > > > __free(put_device) and guard(device), define the same for PCI devices, > > > > __free(pci_dev_put) and guard(pci_dev). These helpers eliminate the > > > > need for "goto free;" and "goto unlock;" patterns. For example, A > > > > 'struct pci_dev *' instance declared as: > > > > > > > > struct pci_dev *pdev __free(pci_dev_put) = NULL; > > > > > > I see several similar __free() uses with NULL initializations in gpio, > > > but I think this idiom would be slightly improved if the __free() > > > function were more closely associated with the actual pci_dev_get(): > > > > > > struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...); > > > > > > Not always possible, I know, but easier to analyze when it is. > > > > I tend to agree, but it does lead to some long lines, for example: > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 4fd1f207c84e..549ba4b8294e 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > > struct cxl_cper_event_rec *rec) > > { > > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > > enum cxl_event_log_type log_type; > > struct cxl_dev_state *cxlds; > > unsigned int devfn; > > u32 hdr_flags; > > > > devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > > - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, > > - device_id->bus_num, devfn); > > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > > + device_id->segment_num, device_id->bus_num, devfn); > > if (!pdev) > > return; > > > > ...so I think people are choosing the "... __free(x) = NULL;" style for > > code density readability. > > > > Also in this case we need devfn assigned first. > > Is the above patch compliant with current style guidelines? > > Or would it be better to do? > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index b14237f824cf..8a180c6abb67 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > struct cxl_cper_event_rec *rec) > { > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > enum cxl_event_log_type log_type; > struct cxl_dev_state *cxlds; > - unsigned int devfn; > + unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > + device_id->segment_num, > + device_id->bus_num, devfn); I don't really care about this specific instance; my comment was more about the commit log for the "Define scope based management functions" patch, thinking maybe the example could encourage get/put togetherness when it's practical. Bjorn
Bjorn Helgaas wrote: > On Thu, Jan 04, 2024 at 01:46:56PM -0800, Ira Weiny wrote: > > Dan Williams wrote: > > > Bjorn Helgaas wrote: [snip] > > > > Also in this case we need devfn assigned first. > > > > Is the above patch compliant with current style guidelines? > > > > Or would it be better to do? > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index b14237f824cf..8a180c6abb67 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type, > > struct cxl_cper_event_rec *rec) > > { > > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > > enum cxl_event_log_type log_type; > > struct cxl_dev_state *cxlds; > > - unsigned int devfn; > > + unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > > + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot( > > + device_id->segment_num, > > + device_id->bus_num, devfn); > > I don't really care about this specific instance; my comment was more > about the commit log for the "Define scope based management functions" > patch, thinking maybe the example could encourage get/put togetherness > when it's practical. Ok thanks! Ira
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(+)