Re: [PATCH 7/7] cxl/region: Recycle region ids

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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