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

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

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

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, &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.

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.williams@intel.com>

Thanks!