| Message ID | 166752186062.947915.13200195701224993317.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | 6713c47b312cd896d940700521a806e74adb54de |
| Headers | show |
| Series | CXL region creation fixes for 6.1 | expand |
On Thu, 2022-11-03 at 17:31 -0700, Dan Williams wrote: > At region creation time the next region-id is atomically cached so that > there is predictability of region device names. If that region is > destroyed and then a new one is created the region id increments. That > ends up looking like a memory leak, or is otherwise surprising that > identifiers roll forward even after destroying all previously created > regions. > > Try to reuse rather than free old region ids at region release time. > > While this fixes a cosmetic issue, the needlessly advancing memory > region-id gives the appearance of a memory leak, hence the "Fixes" tag, > but no "Cc: stable" tag. > > Cc: Ben Widawsky <bwidawsk@kernel.org> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/region.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) Makes sense, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c0253de74945..f9ae5ad284ff 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1534,9 +1534,24 @@ static const struct attribute_group *region_groups[] = { > > static void cxl_region_release(struct device *dev) > { > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent); > struct cxl_region *cxlr = to_cxl_region(dev); > + int id = atomic_read(&cxlrd->region_id); > + > + /* > + * Try to reuse the recently idled id rather than the cached > + * next id to prevent the region id space from increasing > + * unnecessarily. > + */ > + if (cxlr->id < id) > + if (atomic_try_cmpxchg(&cxlrd->region_id, &id, cxlr->id)) { > + memregion_free(id); > + goto out; > + } > > memregion_free(cxlr->id); > +out: > + put_device(dev->parent); > kfree(cxlr); > } > > @@ -1598,6 +1613,11 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i > device_initialize(dev); > lockdep_set_class(&dev->mutex, &cxl_region_key); > dev->parent = &cxlrd->cxlsd.cxld.dev; > + /* > + * Keep root decoder pinned through cxl_region_release to fixup > + * region id allocations > + */ > + get_device(dev->parent); > device_set_pm_not_required(dev); > dev->bus = &cxl_bus_type; > dev->type = &cxl_region_type; >
On 11/3/2022 5:31 PM, Dan Williams wrote: > At region creation time the next region-id is atomically cached so that > there is predictability of region device names. If that region is > destroyed and then a new one is created the region id increments. That > ends up looking like a memory leak, or is otherwise surprising that > identifiers roll forward even after destroying all previously created > regions. > > Try to reuse rather than free old region ids at region release time. > > While this fixes a cosmetic issue, the needlessly advancing memory > region-id gives the appearance of a memory leak, hence the "Fixes" tag, > but no "Cc: stable" tag. > > Cc: Ben Widawsky <bwidawsk@kernel.org> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c0253de74945..f9ae5ad284ff 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1534,9 +1534,24 @@ static const struct attribute_group *region_groups[] = { > > static void cxl_region_release(struct device *dev) > { > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent); > struct cxl_region *cxlr = to_cxl_region(dev); > + int id = atomic_read(&cxlrd->region_id); > + > + /* > + * Try to reuse the recently idled id rather than the cached > + * next id to prevent the region id space from increasing > + * unnecessarily. > + */ > + if (cxlr->id < id) > + if (atomic_try_cmpxchg(&cxlrd->region_id, &id, cxlr->id)) { > + memregion_free(id); > + goto out; > + } > > memregion_free(cxlr->id); > +out: > + put_device(dev->parent); > kfree(cxlr); > } > > @@ -1598,6 +1613,11 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i > device_initialize(dev); > lockdep_set_class(&dev->mutex, &cxl_region_key); > dev->parent = &cxlrd->cxlsd.cxld.dev; > + /* > + * Keep root decoder pinned through cxl_region_release to fixup > + * region id allocations > + */ > + get_device(dev->parent); > device_set_pm_not_required(dev); > dev->bus = &cxl_bus_type; > dev->type = &cxl_region_type; >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index c0253de74945..f9ae5ad284ff 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1534,9 +1534,24 @@ static const struct attribute_group *region_groups[] = { static void cxl_region_release(struct device *dev) { + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent); struct cxl_region *cxlr = to_cxl_region(dev); + int id = atomic_read(&cxlrd->region_id); + + /* + * Try to reuse the recently idled id rather than the cached + * next id to prevent the region id space from increasing + * unnecessarily. + */ + if (cxlr->id < id) + if (atomic_try_cmpxchg(&cxlrd->region_id, &id, cxlr->id)) { + memregion_free(id); + goto out; + } memregion_free(cxlr->id); +out: + put_device(dev->parent); kfree(cxlr); } @@ -1598,6 +1613,11 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i device_initialize(dev); lockdep_set_class(&dev->mutex, &cxl_region_key); dev->parent = &cxlrd->cxlsd.cxld.dev; + /* + * Keep root decoder pinned through cxl_region_release to fixup + * region id allocations + */ + get_device(dev->parent); device_set_pm_not_required(dev); dev->bus = &cxl_bus_type; dev->type = &cxl_region_type;
At region creation time the next region-id is atomically cached so that there is predictability of region device names. If that region is destroyed and then a new one is created the region id increments. That ends up looking like a memory leak, or is otherwise surprising that identifiers roll forward even after destroying all previously created regions. Try to reuse rather than free old region ids at region release time. While this fixes a cosmetic issue, the needlessly advancing memory region-id gives the appearance of a memory leak, hence the "Fixes" tag, but no "Cc: stable" tag. Cc: Ben Widawsky <bwidawsk@kernel.org> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)