| Message ID | 20220705232159.2218958-2-ira.weiny@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Introduce devm_xa_init | expand |
On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Many devices may have arrays of resources which are allocated with > device managed functions. The objects referenced by the XArray are > therefore automatically destroyed without the need for the XArray. "... without the need for the XArray" seems like it's missing something. Should this say something like "... without the need for destroying them in the XArray destroy action"? > Introduce devm_xa_init() which takes care of the destruction of the > XArray meta data automatically as well. > > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > The main issue I see with this is defining devm_xa_init() in device.h. > This makes sense because a device is required to use the call. However, > I'm worried about if users will find the call there vs including it in > xarray.h? > --- > drivers/base/core.c | 20 ++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 2eede2ec3d64..8c5c20a62744 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -2609,6 +2609,26 @@ void devm_device_remove_groups(struct device *dev, > } > EXPORT_SYMBOL_GPL(devm_device_remove_groups); > > +static void xa_destroy_cb(void *xa) > +{ > + xa_destroy(xa); > +} > + > +/** > + * devm_xa_init() - Device managed initialization of an empty XArray > + * @dev: The device this xarray is associated with > + * @xa: XArray > + * > + * Context: Any context > + * Returns: 0 on success, -errno if the action fails to be set > + */ > +int devm_xa_init(struct device *dev, struct xarray *xa) > +{ > + xa_init(xa); > + return devm_add_action(dev, xa_destroy_cb, xa); > +} > +EXPORT_SYMBOL(devm_xa_init); > + > static int device_add_attrs(struct device *dev) > { > struct class *class = dev->class; > diff --git a/include/linux/device.h b/include/linux/device.h > index 073f1b0126ac..e06dc63e375b 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -27,6 +27,7 @@ > #include <linux/uidgid.h> > #include <linux/gfp.h> > #include <linux/overflow.h> > +#include <linux/xarray.h> > #include <linux/device/bus.h> > #include <linux/device/class.h> > #include <linux/device/driver.h> > @@ -978,6 +979,8 @@ int __must_check devm_device_add_group(struct device *dev, > void devm_device_remove_group(struct device *dev, > const struct attribute_group *grp); > > +int devm_xa_init(struct device *dev, struct xarray *xa); > + > /* > * Platform "fixup" functions - allow the platform to have their say > * about devices and actions that the general device layer doesn't > -- > 2.35.3 >
On Thu, Jul 07, 2022 at 11:10:47AM -0500, Bjorn Helgaas wrote: > On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Many devices may have arrays of resources which are allocated with > > device managed functions. The objects referenced by the XArray are > > therefore automatically destroyed without the need for the XArray. > > "... without the need for the XArray" seems like it's missing > something. > > Should this say something like "... without the need for destroying > them in the XArray destroy action"? Yes that is true. But what I was trying to say was that the objects have a built in alias in the device managed infrastructure which will be used to free that memory. So the pointers stored in the XArray are not needed to destroy them directly; for example by iterating through them with xa_for_each(). Thus the "without the need for the XArray". I'll try and clarify more for V1. So far it does not seem like there is any opposition to this but I'll give it a few more days for anyone to object. Ira > > > Introduce devm_xa_init() which takes care of the destruction of the > > XArray meta data automatically as well. > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Matthew Wilcox <willy@infradead.org> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > --- > > The main issue I see with this is defining devm_xa_init() in device.h. > > This makes sense because a device is required to use the call. However, > > I'm worried about if users will find the call there vs including it in > > xarray.h? > > --- > > drivers/base/core.c | 20 ++++++++++++++++++++ > > include/linux/device.h | 3 +++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 2eede2ec3d64..8c5c20a62744 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -2609,6 +2609,26 @@ void devm_device_remove_groups(struct device *dev, > > } > > EXPORT_SYMBOL_GPL(devm_device_remove_groups); > > > > +static void xa_destroy_cb(void *xa) > > +{ > > + xa_destroy(xa); > > +} > > + > > +/** > > + * devm_xa_init() - Device managed initialization of an empty XArray > > + * @dev: The device this xarray is associated with > > + * @xa: XArray > > + * > > + * Context: Any context > > + * Returns: 0 on success, -errno if the action fails to be set > > + */ > > +int devm_xa_init(struct device *dev, struct xarray *xa) > > +{ > > + xa_init(xa); > > + return devm_add_action(dev, xa_destroy_cb, xa); > > +} > > +EXPORT_SYMBOL(devm_xa_init); > > + > > static int device_add_attrs(struct device *dev) > > { > > struct class *class = dev->class; > > diff --git a/include/linux/device.h b/include/linux/device.h > > index 073f1b0126ac..e06dc63e375b 100644 > > --- a/include/linux/device.h > > +++ b/include/linux/device.h > > @@ -27,6 +27,7 @@ > > #include <linux/uidgid.h> > > #include <linux/gfp.h> > > #include <linux/overflow.h> > > +#include <linux/xarray.h> > > #include <linux/device/bus.h> > > #include <linux/device/class.h> > > #include <linux/device/driver.h> > > @@ -978,6 +979,8 @@ int __must_check devm_device_add_group(struct device *dev, > > void devm_device_remove_group(struct device *dev, > > const struct attribute_group *grp); > > > > +int devm_xa_init(struct device *dev, struct xarray *xa); > > + > > /* > > * Platform "fixup" functions - allow the platform to have their say > > * about devices and actions that the general device layer doesn't > > -- > > 2.35.3 > >
On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > The main issue I see with this is defining devm_xa_init() in device.h. > This makes sense because a device is required to use the call. However, > I'm worried about if users will find the call there vs including it in > xarray.h? Honestly, I don't want users to find it. This only makes sense if you're already bought in to the devm cult. I worry people will think that they don't need to do anything else; that everything will be magically freed for them, and we'll leak the objects pointed to from the xarray. I don't even like having xa_destroy() in the API, because of exactly this.
On Fri, Jul 08, 2022 at 03:53:50PM +0100, Matthew Wilcox wrote: > On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > > The main issue I see with this is defining devm_xa_init() in device.h. > > This makes sense because a device is required to use the call. However, > > I'm worried about if users will find the call there vs including it in > > xarray.h? > > Honestly, I don't want users to find it. This only makes sense if you're > already bought in to the devm cult. I worry people will think that > they don't need to do anything else; that everything will be magically > freed for them, and we'll leak the objects pointed to from the xarray. > I don't even like having xa_destroy() in the API, because of exactly this. > Fair enough. Are you ok with the concept though? Ira
On Fri, Jul 08, 2022 at 07:59:22AM -0700, Ira Weiny wrote: > On Fri, Jul 08, 2022 at 03:53:50PM +0100, Matthew Wilcox wrote: > > On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > > > The main issue I see with this is defining devm_xa_init() in device.h. > > > This makes sense because a device is required to use the call. However, > > > I'm worried about if users will find the call there vs including it in > > > xarray.h? > > > > Honestly, I don't want users to find it. This only makes sense if you're > > already bought in to the devm cult. I worry people will think that > > they don't need to do anything else; that everything will be magically > > freed for them, and we'll leak the objects pointed to from the xarray. > > I don't even like having xa_destroy() in the API, because of exactly this. > > > > Fair enough. Are you ok with the concept though? I'd rather have it in one place than open-coded in two.
Ira Weiny wrote: > On Fri, Jul 08, 2022 at 03:53:50PM +0100, Matthew Wilcox wrote: > > On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > > > The main issue I see with this is defining devm_xa_init() in device.h. > > > This makes sense because a device is required to use the call. However, > > > I'm worried about if users will find the call there vs including it in > > > xarray.h? > > > > Honestly, I don't want users to find it. This only makes sense if you're > > already bought in to the devm cult. I worry people will think that > > they don't need to do anything else; that everything will be magically > > freed for them, and we'll leak the objects pointed to from the xarray. > > I don't even like having xa_destroy() in the API, because of exactly this. > > > > Fair enough. Are you ok with the concept though? I came here to same the same thing as Matthew. devm_xa_init() does not lessen review burden like other devm helpers. A reviewer still needs to go verfy that the patch that uses this makes sure to free all objects in the xarray before it gets destroyed. If there still needs to be an open-coded "empty the xarray" step, then that can just do the xa_destroy() there. So for me, no, the concept of this just not quite jive.
On Thu, Jul 14, 2022 at 08:44:00AM -0700, Dan Williams wrote: > Ira Weiny wrote: > > On Fri, Jul 08, 2022 at 03:53:50PM +0100, Matthew Wilcox wrote: > > > On Tue, Jul 05, 2022 at 04:21:57PM -0700, ira.weiny@intel.com wrote: > > > > The main issue I see with this is defining devm_xa_init() in device.h. > > > > This makes sense because a device is required to use the call. However, > > > > I'm worried about if users will find the call there vs including it in > > > > xarray.h? > > > > > > Honestly, I don't want users to find it. This only makes sense if you're > > > already bought in to the devm cult. I worry people will think that > > > they don't need to do anything else; that everything will be magically > > > freed for them, and we'll leak the objects pointed to from the xarray. > > > I don't even like having xa_destroy() in the API, because of exactly this. > > > > > > > Fair enough. Are you ok with the concept though? > > I came here to same the same thing as Matthew. devm_xa_init() does not > lessen review burden like other devm helpers. A reviewer still needs to > go verfy that the patch that uses this makes sure to free all objects in > the xarray before it gets destroyed. > > If there still needs to be an open-coded "empty the xarray" step, then > that can just do the xa_destroy() there. So for me, no, the concept of > this just not quite jive. Ok I'll drop it. Ira
diff --git a/drivers/base/core.c b/drivers/base/core.c index 2eede2ec3d64..8c5c20a62744 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2609,6 +2609,26 @@ void devm_device_remove_groups(struct device *dev, } EXPORT_SYMBOL_GPL(devm_device_remove_groups); +static void xa_destroy_cb(void *xa) +{ + xa_destroy(xa); +} + +/** + * devm_xa_init() - Device managed initialization of an empty XArray + * @dev: The device this xarray is associated with + * @xa: XArray + * + * Context: Any context + * Returns: 0 on success, -errno if the action fails to be set + */ +int devm_xa_init(struct device *dev, struct xarray *xa) +{ + xa_init(xa); + return devm_add_action(dev, xa_destroy_cb, xa); +} +EXPORT_SYMBOL(devm_xa_init); + static int device_add_attrs(struct device *dev) { struct class *class = dev->class; diff --git a/include/linux/device.h b/include/linux/device.h index 073f1b0126ac..e06dc63e375b 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -27,6 +27,7 @@ #include <linux/uidgid.h> #include <linux/gfp.h> #include <linux/overflow.h> +#include <linux/xarray.h> #include <linux/device/bus.h> #include <linux/device/class.h> #include <linux/device/driver.h> @@ -978,6 +979,8 @@ int __must_check devm_device_add_group(struct device *dev, void devm_device_remove_group(struct device *dev, const struct attribute_group *grp); +int devm_xa_init(struct device *dev, struct xarray *xa); + /* * Platform "fixup" functions - allow the platform to have their say * about devices and actions that the general device layer doesn't