[RFC PATCH 0/3] Introduce devm_xa_init

[RFC PATCH 0/3] Introduce devm_xa_init

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

This is submitted RFC for 2 reasons.  First I'm not quite sure where to place
the call in the headers.  Second the use of the new call is dependent on some
CXL code which was just been submitted.[0]  I want to get opinions on if this new
call seems useful or just more confusing to the XArray interface.  If useful
I'll respin after the CXL stuff lands and perhaps it can go through Dan's tree.

While converting some CXL code to XArray a pattern emerged which seemed useful
to codify.

In two different situations[1][2] an XArray was initialized in such a way that
using devm_add_action() could be used to call xa_destroy() automatically.

In the first situation[1] the XArray was storing long values directly and in
the other situation the pointers were allocated using device managed functions
(devm_*).

In these situations it seems that a device managed xa_init() would be useful.

[0] https://lore.kernel.org/linux-cxl/20220705154932.2141021-1-ira.weiny@intel.com/
[1] https://lore.kernel.org/linux-cxl/20220705154932.2141021-4-ira.weiny@intel.com/
[2] https://lore.kernel.org/linux-cxl/20220705154932.2141021-5-ira.weiny@intel.com/



Ira Weiny (3):
  xarray: Introduce devm_xa_init()
  pci/doe: Use devm_xa_init()
  CXL/doe: Use devm_xa_init()

 drivers/base/core.c    | 20 ++++++++++++++++++++
 drivers/cxl/pci.c      |  8 +-------
 drivers/pci/doe.c      | 14 ++------------
 include/linux/device.h |  3 +++
 4 files changed, 26 insertions(+), 19 deletions(-)

-- 
2.35.3

[RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[ira.weiny]

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.

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

[RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The XArray being used to store the protocols does not even store
allocated objects.

Use devm_xa_init() to automatically destroy the XArray when the PCI
device goes away.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/pci/doe.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 0b02f33ef994..aa36f459d375 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -386,13 +386,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
 	return 0;
 }
 
-static void pci_doe_xa_destroy(void *mb)
-{
-	struct pci_doe_mb *doe_mb = mb;
-
-	xa_destroy(&doe_mb->prots);
-}
-
 static void pci_doe_destroy_workqueue(void *mb)
 {
 	struct pci_doe_mb *doe_mb = mb;
@@ -440,11 +433,8 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
 	doe_mb->pdev = pdev;
 	doe_mb->cap_offset = cap_offset;
 	init_waitqueue_head(&doe_mb->wq);
-
-	xa_init(&doe_mb->prots);
-	rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb);
-	if (rc)
-		return ERR_PTR(rc);
+	if (devm_xa_init(dev, &doe_mb->prots))
+		return ERR_PTR(-ENOMEM);
 
 	doe_mb->work_queue = alloc_ordered_workqueue("DOE: [%x]", 0,
 						     doe_mb->cap_offset);
-- 
2.35.3

[RFC PATCH 3/3] CXL/doe: Use devm_xa_init()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The DOE mailboxes are all allocated with device managed calls.
Therefore, the XArray can go away when the device goes away.

Use devm_xa_init() and remove the callback needed for xa_destroy().

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/pci.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 6228c95fd142..adb8198fc6ad 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -387,11 +387,6 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 	return rc;
 }
 
-static void cxl_pci_destroy_doe(void *mbs)
-{
-	xa_destroy(mbs);
-}
-
 static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
 {
 	struct device *dev = cxlds->dev;
@@ -446,8 +441,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlds))
 		return PTR_ERR(cxlds);
 
-	xa_init(&cxlds->doe_mbs);
-	if (devm_add_action(&pdev->dev, cxl_pci_destroy_doe, &cxlds->doe_mbs))
+	if (devm_xa_init(&pdev->dev, &cxlds->doe_mbs))
 		return -ENOMEM;
 
 	cxlds->serial = pci_get_dsn(pdev);
-- 
2.35.3

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Bjorn Helgaas]

On Tue, Jul 05, 2022 at 04:21:58PM -0700, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> The XArray being used to store the protocols does not even store
> allocated objects.

I guess the point is that the doe_mb->prots XArray doesn't reference
any other objects that would need to be freed when destroying
doe_mb->prots?  A few more words here would make the commit log more
useful to non-XArray experts.

s|pci/doe|PCI/DOE| in subject to match the drivers/pci convention.

> Use devm_xa_init() to automatically destroy the XArray when the PCI
> device goes away.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/pci/doe.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 0b02f33ef994..aa36f459d375 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -386,13 +386,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
>  	return 0;
>  }
>  
> -static void pci_doe_xa_destroy(void *mb)
> -{
> -	struct pci_doe_mb *doe_mb = mb;
> -
> -	xa_destroy(&doe_mb->prots);
> -}
> -
>  static void pci_doe_destroy_workqueue(void *mb)
>  {
>  	struct pci_doe_mb *doe_mb = mb;
> @@ -440,11 +433,8 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
>  	doe_mb->pdev = pdev;
>  	doe_mb->cap_offset = cap_offset;
>  	init_waitqueue_head(&doe_mb->wq);
> -
> -	xa_init(&doe_mb->prots);
> -	rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb);
> -	if (rc)
> -		return ERR_PTR(rc);
> +	if (devm_xa_init(dev, &doe_mb->prots))
> +		return ERR_PTR(-ENOMEM);
>  
>  	doe_mb->work_queue = alloc_ordered_workqueue("DOE: [%x]", 0,
>  						     doe_mb->cap_offset);
> -- 
> 2.35.3
> 

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Bjorn Helgaas]

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
> 

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Ira Weiny]

On Thu, Jul 07, 2022 at 11:06:46AM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 05, 2022 at 04:21:58PM -0700, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The XArray being used to store the protocols does not even store
> > allocated objects.
> 
> I guess the point is that the doe_mb->prots XArray doesn't reference
> any other objects that would need to be freed when destroying
> doe_mb->prots?

Yes.

> A few more words here would make the commit log more
> useful to non-XArray experts.

I'll update this to be more clear in a V1 if it goes that far.  But to clarify
here; the protocol information is a u16 vendor id and u8 protocol number.  So
we are able to store that in the unsigned long value that would normally be a
pointer to something in the XArray.

> 
> s|pci/doe|PCI/DOE| in subject to match the drivers/pci convention.

Yes. Sorry,

Thanks for the review,
Ira

> 
> > Use devm_xa_init() to automatically destroy the XArray when the PCI
> > device goes away.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/pci/doe.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 0b02f33ef994..aa36f459d375 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -386,13 +386,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
> >  	return 0;
> >  }
> >  
> > -static void pci_doe_xa_destroy(void *mb)
> > -{
> > -	struct pci_doe_mb *doe_mb = mb;
> > -
> > -	xa_destroy(&doe_mb->prots);
> > -}
> > -
> >  static void pci_doe_destroy_workqueue(void *mb)
> >  {
> >  	struct pci_doe_mb *doe_mb = mb;
> > @@ -440,11 +433,8 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
> >  	doe_mb->pdev = pdev;
> >  	doe_mb->cap_offset = cap_offset;
> >  	init_waitqueue_head(&doe_mb->wq);
> > -
> > -	xa_init(&doe_mb->prots);
> > -	rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb);
> > -	if (rc)
> > -		return ERR_PTR(rc);
> > +	if (devm_xa_init(dev, &doe_mb->prots))
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	doe_mb->work_queue = alloc_ordered_workqueue("DOE: [%x]", 0,
> >  						     doe_mb->cap_offset);
> > -- 
> > 2.35.3
> > 

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Matthew Wilcox]

On Fri, Jul 08, 2022 at 07:45:12AM -0700, Ira Weiny wrote:
> On Thu, Jul 07, 2022 at 11:06:46AM -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 05, 2022 at 04:21:58PM -0700, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > The XArray being used to store the protocols does not even store
> > > allocated objects.
> > 
> > I guess the point is that the doe_mb->prots XArray doesn't reference
> > any other objects that would need to be freed when destroying
> > doe_mb->prots?
> 
> Yes.
> 
> > A few more words here would make the commit log more
> > useful to non-XArray experts.
> 
> I'll update this to be more clear in a V1 if it goes that far.  But to clarify
> here; the protocol information is a u16 vendor id and u8 protocol number.  So
> we are able to store that in the unsigned long value that would normally be a
> pointer to something in the XArray.

Er.  Signed long.  I can't find drivers/pci/doe.c in linux-next, so
I have no idea if you're doing something wrong.  But what you said here
sounds wrong.

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Ira Weiny]

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

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Matthew Wilcox]

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.

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Ira Weiny]

On Fri, Jul 08, 2022 at 03:49:51PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 08, 2022 at 07:45:12AM -0700, Ira Weiny wrote:
> > On Thu, Jul 07, 2022 at 11:06:46AM -0500, Bjorn Helgaas wrote:
> > > On Tue, Jul 05, 2022 at 04:21:58PM -0700, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > The XArray being used to store the protocols does not even store
> > > > allocated objects.
> > > 
> > > I guess the point is that the doe_mb->prots XArray doesn't reference
> > > any other objects that would need to be freed when destroying
> > > doe_mb->prots?
> > 
> > Yes.
> > 
> > > A few more words here would make the commit log more
> > > useful to non-XArray experts.
> > 
> > I'll update this to be more clear in a V1 if it goes that far.  But to clarify
> > here; the protocol information is a u16 vendor id and u8 protocol number.  So
> > we are able to store that in the unsigned long value that would normally be a
> > pointer to something in the XArray.
> 
> Er.  Signed long.

Sorry I misspoke, xa_mk_value() takes an unsigned long.

> I can't find drivers/pci/doe.c in linux-next, so
> I have no idea if you're doing something wrong.

Sorry doe.c does not exist yet.  I came up with this idea while developing a
CXL series which is still in review.[0]

> But what you said here
> sounds wrong.

:-/

Can't I use xa_mk_value() to store data directly in the entry "pointer"?

From patch 3/9 in that series.[1]

+static void *pci_doe_xa_prot_entry(u16 vid, u8 prot)
+{
+	return xa_mk_value(((unsigned long)vid << 16) | prot);
+}

Both Dan and I thought this was acceptable in XArray?

Ira

[0] https://lore.kernel.org/linux-cxl/20220705154932.2141021-1-ira.weiny@intel.com/
[1] https://lore.kernel.org/linux-cxl/20220705154932.2141021-4-ira.weiny@intel.com/

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Ira Weiny]

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

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Matthew Wilcox]

On Fri, Jul 08, 2022 at 07:57:10AM -0700, Ira Weiny wrote:
> > > I'll update this to be more clear in a V1 if it goes that far.  But to clarify
> > > here; the protocol information is a u16 vendor id and u8 protocol number.  So
> > > we are able to store that in the unsigned long value that would normally be a
> > > pointer to something in the XArray.
> > 
> > Er.  Signed long.
> 
> Sorry I misspoke, xa_mk_value() takes an unsigned long.

It does, *but* ...

static inline void *xa_mk_value(unsigned long v)
{
        WARN_ON((long)v < 0);
        return (void *)((v << 1) | 1);
}

... you can't pass an integer that has the top bit set to it.

> Can't I use xa_mk_value() to store data directly in the entry "pointer"?

Yes, that's the purpose of xa_mk_value().  From what you said, it sounded
like you were just storing the integer directly, which won't work.

> +static void *pci_doe_xa_prot_entry(u16 vid, u8 prot)
> +{
> +	return xa_mk_value(((unsigned long)vid << 16) | prot);
> +}
> 
> Both Dan and I thought this was acceptable in XArray?

You haven't tested that on 32-bit, have you?  Shift vid by 8 instead of
16, and it'll be fine.

(Oh, and you don't need to cast vid; the standard C integer promotions
will promote vid to int before shifting, and you won't lose any bits)

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Matthew Wilcox]

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.

Re: [RFC PATCH 2/3] pci/doe: Use devm_xa_init()

[Ira Weiny]

On Fri, Jul 08, 2022 at 04:04:05PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 08, 2022 at 07:57:10AM -0700, Ira Weiny wrote:
> > > > I'll update this to be more clear in a V1 if it goes that far.  But to clarify
> > > > here; the protocol information is a u16 vendor id and u8 protocol number.  So
> > > > we are able to store that in the unsigned long value that would normally be a
> > > > pointer to something in the XArray.
> > > 
> > > Er.  Signed long.
> > 
> > Sorry I misspoke, xa_mk_value() takes an unsigned long.
> 
> It does, *but* ...
> 
> static inline void *xa_mk_value(unsigned long v)
> {
>         WARN_ON((long)v < 0);
>         return (void *)((v << 1) | 1);
> }
> 
> ... you can't pass an integer that has the top bit set to it.
> 
> > Can't I use xa_mk_value() to store data directly in the entry "pointer"?
> 
> Yes, that's the purpose of xa_mk_value().  From what you said, it sounded
> like you were just storing the integer directly, which won't work.
> 
> > +static void *pci_doe_xa_prot_entry(u16 vid, u8 prot)
> > +{
> > +	return xa_mk_value(((unsigned long)vid << 16) | prot);
> > +}
> > 
> > Both Dan and I thought this was acceptable in XArray?
> 
> You haven't tested that on 32-bit, have you?  Shift vid by 8 instead of
> 16, and it'll be fine.

Ah ok.

> 
> (Oh, and you don't need to cast vid; the standard C integer promotions
> will promote vid to int before shifting, and you won't lose any bits)

Will do, thanks!
Ira

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Dan Williams]

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.

Re: [RFC PATCH 1/3] xarray: Introduce devm_xa_init()

[Ira Weiny]

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