| Message ID | 20220705232159.2218958-3-ira.weiny@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Introduce devm_xa_init | expand |
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 >
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 > >
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.
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/
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 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
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);