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.willi...@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.ve...@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, ®ion->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.willi...@intel.com>
