From: Ira Weiny <[email protected]>
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/[email protected]/
[1] https://lore.kernel.org/linux-cxl/[email protected]/
[2] https://lore.kernel.org/linux-cxl/[email protected]/
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
From: Ira Weiny <[email protected]>
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 <[email protected]>
---
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
From: Ira Weiny <[email protected]>
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 <[email protected]>
---
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
On Tue, Jul 05, 2022 at 04:21:58PM -0700, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
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, [email protected] wrote:
> > > From: Ira Weiny <[email protected]>
> > >
> > > 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.
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, [email protected] wrote:
> > > > From: Ira Weiny <[email protected]>
> > > >
> > > > 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/[email protected]/
[1] https://lore.kernel.org/linux-cxl/[email protected]/
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)
On Thu, Jul 07, 2022 at 11:06:46AM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 05, 2022 at 04:21:58PM -0700, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> >
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