On Thu, 2022-08-11 at 13:22 -0700, Dan Williams wrote:
> 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;
I didn't add it because list_add_sorted didn't seem to do anything with
the equal case, but yeah I think for a compare helper, it makes sense
to be complete.
>
> > +}
> > +
> > 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.
Yep I got greedy with the trickery, but keeping it idiomatic makes more
sense.
>
> > + 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:
Yep all other comments sound good.
> Reviewed-by: Dan Williams <dan.j.willi...@intel.com>
Thanks!
