RE: [ndctl PATCH v2 10/10] cxl/decoder: add a max_available_extent attribute

From: Dan Williams <dan.j.williams@intel.com>
To: Vishal Verma <vishal.l.verma@intel.com>, <linux-cxl@vger.kernel.org>
Cc: <nvdimm@lists.linux.dev>, Dan Williams <dan.j.williams@intel.com>,
	"Alison Schofield" <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: RE: [ndctl PATCH v2 10/10] cxl/decoder: add a max_available_extent attribute
Date: Thu, 11 Aug 2022 13:22:45 -0700	[thread overview]
Message-ID: <62f56515c9f10_3ce68294ea@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <20220810230914.549611-11-vishal.l.verma@intel.com>

Vishal Verma wrote:
> Add a max_available_extent attribute to cxl_decoder. In order to aid in
> its calculation, change the order of regions in the root decoder's list
> to be sorted by start HPA of the region.
> 
> Additionally, emit this attribute in decoder listings, and consult it
> for available space before creating a new region.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/lib/private.h  |  1 +
>  cxl/lib/libcxl.c   | 86 +++++++++++++++++++++++++++++++++++++++++++++-
>  cxl/libcxl.h       |  3 ++
>  cxl/json.c         |  8 +++++
>  cxl/region.c       | 14 +++++++-
>  cxl/lib/libcxl.sym |  1 +
>  6 files changed, 111 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index 8619bb1..8705e46 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -104,6 +104,7 @@ struct cxl_decoder {
>  	u64 size;
>  	u64 dpa_resource;
>  	u64 dpa_size;
> +	u64 max_available_extent;
>  	void *dev_buf;
>  	size_t buf_len;
>  	char *dev_path;
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index b4d7890..3c1a2c3 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -446,6 +446,11 @@ CXL_EXPORT int cxl_region_delete(struct cxl_region *region)
>  	return 0;
>  }
>  
> +static int region_start_cmp(struct cxl_region *r1, struct cxl_region *r2)
> +{
> +	return ((r1->start < r2->start) ? -1 : 1);

I think you want 'equal' case too, right?

val = r1->start - r2->start;
if (val > r1->start)
	return -1;
if (val < r1->start)
	return 1;
return 0;

> +}
> +
>  static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  {
>  	const char *devname = devpath_to_devname(cxlregion_base);
> @@ -528,7 +533,7 @@ static void *add_cxl_region(void *parent, int id, const char *cxlregion_base)
>  			return region_dup;
>  		}
>  
> -	list_add(&decoder->regions, &region->list);
> +	list_add_sorted(&decoder->regions, region, list, region_start_cmp);
>  
>  	return region;
>  err:
> @@ -1606,6 +1611,74 @@ cxl_endpoint_get_memdev(struct cxl_endpoint *endpoint)
>  	return NULL;
>  }
>  
> +static int cxl_region_is_configured(struct cxl_region *region)

s/int/bool/

> +{
> +	if ((region->start == 0) && (region->size == 0) &&
> +	    (region->decode_state == CXL_DECODE_RESET))
> +		return 0;
> +	return 1;

That can be squished to just:

	return region->size && region->decode_state != CXL_DECODE_RESET;

...becase region->start == 0 is a valid state for a configured region.

> +}
> +
> +/**
> + * cxl_decoder_calc_max_available_extent() - calculate max available free space
> + * @decoder - the root decoder to calculate the free extents for
> + *
> + * The add_cxl_region() function  adds regions to the parent decoder's list
> + * sorted by the region's start HPAs. It can also be assumed that regions have
> + * no overlapped / aliased HPA space. Therefore, calculating each extent is as
> + * simple as walking the region list in order, and subtracting the previous
> + * region's end HPA from the next region's start HPA (and taking into account
> + * the decoder's start and end HPAs as well).
> + */
> +static unsigned long long
> +cxl_decoder_calc_max_available_extent(struct cxl_decoder *decoder)
> +{
> +	struct cxl_port *port = cxl_decoder_get_port(decoder);
> +	struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder);
> +	u64 prev_end = 0, max_extent = 0;
> +	struct cxl_region *region;
> +
> +	if (!cxl_port_is_root(port)) {
> +		err(ctx, "%s: not a root decoder\n",
> +		    cxl_decoder_get_devname(decoder));
> +		return ULLONG_MAX;
> +	}
> +
> +	/*
> +	 * Preload prev_end with decoder's start, so that the extent
> +	 * calculation for the first region Just Works
> +	 */
> +	prev_end = decoder->start;
> +
> +	cxl_region_foreach(decoder, region) {
> +		if (!cxl_region_is_configured(region))
> +			continue;
> +
> +		/*
> +		 * Note: Normally, end = (start + size - 1), but since
> +		 * this is calculating extents between regions, it can
> +		 * skip the '- 1'. For example, if a region ends at 0xfff,
> +		 * and the next region immediately starts at 0x1000,
> +		 * the 'extent' between them is 0, not 1. With
> +		 * end = (start + size), this new 'adjusted' end for the
> +		 * first region will have been calculated as 0x1000.
> +		 * Subtracting the next region's start (0x1000) from this
> +		 * correctly gets the extent size as 0.
> +		 */

Not sure if I prefer this block comment, or just seeing prev_end being
done with -1 math and max_extent doing the + 1 because it's a size...

The latter seems more idiomatic to my eyes, but I'll leave it up to you.

> +		max_extent = max(max_extent, region->start - prev_end);
> +		prev_end = region->start + region->size;
> +	}
> +
> +	/*
> +	 * Finally, consider the extent after the last region, up to the end
> +	 * of the decoder's address space, if any. If there were no regions,
> +	 * this simply reduces to decoder->size.
> +	 */
> +	max_extent = max(max_extent, decoder->start + decoder->size - prev_end);
> +
> +	return max_extent;
> +}
> +
>  static int decoder_id_cmp(struct cxl_decoder *d1, struct cxl_decoder *d2)
>  {
>  	return d1->id - d2->id;
> @@ -1735,6 +1808,8 @@ static void *add_cxl_decoder(void *parent, int id, const char *cxldecoder_base)
>  			if (sysfs_read_attr(ctx, path, buf) == 0)
>  				*(flag->flag) = !!strtoul(buf, NULL, 0);
>  		}
> +		decoder->max_available_extent =
> +			cxl_decoder_calc_max_available_extent(decoder);
>  		break;
>  	}
>  	}
> @@ -1899,6 +1974,12 @@ cxl_decoder_get_dpa_size(struct cxl_decoder *decoder)
>  	return decoder->dpa_size;
>  }
>  
> +CXL_EXPORT unsigned long long
> +cxl_decoder_get_max_available_extent(struct cxl_decoder *decoder)
> +{
> +	return decoder->max_available_extent;
> +}
> +
>  CXL_EXPORT int cxl_decoder_set_dpa_size(struct cxl_decoder *decoder,
>  					unsigned long long size)
>  {
> @@ -2053,6 +2134,9 @@ cxl_decoder_create_pmem_region(struct cxl_decoder *decoder)
>  		return NULL;
>  	}
>  
> +	/* Force a re-init of regions so that the new one can be discovered */
> +	free_regions(decoder);

You do not need to free them, the re-init will do dup detection and
*should* just result in the new one being added. In fact, you probably
do not *want* to free them as that could cause problems for library
users that were holding a 'struct cxl_region' object before the call to
cxl_decoder_create_pmem_region().

With the above fixes folded in you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>