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

[Dave Jiang <dave.jiang@intel.com>]

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