Re: [RFC PATCH 1/4] cxl/extent: Promote cxlr_dax->region_extent to an xarray
From: Dave Jiang
Date: Thu Apr 23 2026 - 20:52:00 EST
On 4/23/26 4:52 PM, John Groves wrote:
> From: John Groves <John@xxxxxxxxxx>
>
> Terminology note: "CXL region" (struct cxl_region) is the
> mode-agnostic HDM-decoded HPA window; "DAX region"
> (struct cxl_dax_region, nicknamed cxlr_dax) is the DCD-specific
> CXL->device-dax shim that hangs off a cxl_region. This commit
> touches only the DAX-region shim; nothing about cxl_region, its
> HDM decoding, or its lifecycle changes.
>
> Prior to this commit, struct cxl_dax_region holds a single
> region_extent pointer, so at most one tagged allocation can ever
> surface under a DAX region. A subsequent commit rewrites DCD
> add-capacity handling to assemble extents per-tag and permits multiple
> allocations per More-chain; that work is latent until cxlr_dax can
> actually hold more than one region_extent. Do the infrastructural
> swap here, on its own, so the behavior change lands in a dedicated
> commit and the per-tag assembly work isn't tangled with storage-layout
> changes.
>
> Replace the single-slot pointer with an xarray. Note: the xarray is
> *not* keyed by UUID. xarray is a radix tree over unsigned-long keys;
> a 128-bit pseudo-random UUID is neither the right width nor the right
> distribution for a radix index. The key is an allocator-assigned u32
> obtained via xa_alloc() when online_region_extent() registers the
> device; that same u32 is stored in region_extent->dev.id so device
> names become "extent<rid>.<id>" with unique suffixes per allocation
> (replacing the hardcoded .0). UUID remains a stored attribute on
> struct region_extent; tag-based lookup is a linear xa_for_each walk,
> which is adequate at the bounded per-region tag counts DCD delivers.
>
> User-visible naming: device names remain "extent<rid>.<N>". Before
> this commit, N is always 0; after, N is the xa_alloc()-assigned id.
> The first region_extent under a cxlr_dax still lands at id 0, so a
> single-allocation region is observably unchanged. Only a 2nd+
> region_extent (which cannot be produced at all today) gets a
> non-zero suffix.
>
> Call-site translations:
>
> - cxl_dax_region_alloc() / _release(): xa_init / xa_destroy.
> - alloc_region_extent(): drop the hardcoded dev.id = 0; the ID is
> assigned when online_region_extent() registers the device.
> - online_region_extent(): device_initialize() first so the release
> function is wired, then xa_alloc(), assign the returned id into
> dev->id, then dev_set_name() and device_add(). On any failure
> from xa_alloc() onward, put_device(dev) -> release ->
> free_region_extent() -> xa_erase() handles cleanup; no explicit
> xa_erase() is needed in the error path of online_region_extent().
> - free_region_extent(): xa_erase() replaces NULLing the slot.
> - extents_contain() / extents_overlap(): nested xa_for_each, outer
> over region_extents, inner over decoder_extents.
> - cxl_rm_extent(): walk region_extents and match by UUID to find
> the target region_extent (linear, bounded).
> - cxl_add_extent()'s "already onlined" gate: translated from
> single-slot NULL check to !xa_empty(). This preserves the
> existing single-region_extent-per-cxlr_dax invariant; lifting
> that gate to actually support multiple allocations is a separate
> semantic change that belongs with the per-tag assembly rework.
>
> The dax-side (drivers/dax/cxl.c) is unchanged. cxl_dax_region_probe()
> still iterates region_extent children via device_for_each_child() on
> cxlr_dax->dev, and cxl_dax_region_notify() still takes a
> struct region_extent * through cxl_notify_data; neither the CXL<->DAX
> boundary nor the DAX resource model is affected.
>
> No functional change: all paths still observe at most one
> region_extent per cxlr_dax; this commit only prepares the storage.
>
> Signed-off-by: John Groves <John@xxxxxxxxxx>
> Signed-off-by: John Groves <john@xxxxxxxxxx>
Double sign offs
> ---
> drivers/cxl/core/extent.c | 89 ++++++++++++++++++++++-----------------
> drivers/cxl/core/region.c | 2 +
> drivers/cxl/cxl.h | 9 +++-
> 3 files changed, 61 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c
> index c4ad81814fb4d..44b58cd477655 100644
> --- a/drivers/cxl/core/extent.c
> +++ b/drivers/cxl/core/extent.c
> @@ -87,7 +87,8 @@ static void free_region_extent(struct region_extent *region_extent)
> xa_for_each(®ion_extent->decoder_extents, index, ed_extent)
> cxled_release_extent(ed_extent->cxled, ed_extent);
> xa_destroy(®ion_extent->decoder_extents);
> - region_extent->cxlr_dax->region_extent = NULL;
> + xa_erase(®ion_extent->cxlr_dax->region_extents,
> + region_extent->dev.id);
> kfree(region_extent);
> }
>
> @@ -144,7 +145,6 @@ alloc_region_extent(struct cxl_dax_region *cxlr_dax, struct range *hpa_range,
> region_extent->hpa_range = *hpa_range;
> region_extent->cxlr_dax = cxlr_dax;
> uuid_copy(®ion_extent->uuid, uuid);
> - region_extent->dev.id = 0;
> xa_init(®ion_extent->decoder_extents);
> return no_free_ptr(region_extent);
> }
> @@ -153,12 +153,26 @@ int online_region_extent(struct region_extent *region_extent)
> {
> struct cxl_dax_region *cxlr_dax = region_extent->cxlr_dax;
> struct device *dev = ®ion_extent->dev;
> + u32 id;
> int rc;
>
> device_initialize(dev);
> device_set_pm_not_required(dev);
> dev->parent = &cxlr_dax->dev;
> dev->type = ®ion_extent_type;
> +
> + /*
> + * Insert into cxlr_dax->region_extents before dev_set_name so
> + * the allocated id is available as the .<N> suffix. On any
> + * failure from here on, put_device(dev) -> release ->
> + * free_region_extent() -> xa_erase() handles the cleanup.
> + */
> + rc = xa_alloc(&cxlr_dax->region_extents, &id, region_extent,
> + xa_limit_32b, GFP_KERNEL);
> + if (rc < 0)
> + goto err;
> + dev->id = id;
> +
> rc = dev_set_name(dev, "extent%d.%d", cxlr_dax->cxlr->id, dev->id);
> if (rc)
> goto err;
> @@ -167,7 +181,6 @@ int online_region_extent(struct region_extent *region_extent)
> if (rc)
> goto err;
>
> - cxlr_dax->region_extent = region_extent;
> dev_dbg(dev, "region extent HPA %pra\n", ®ion_extent->hpa_range);
> return devm_add_action_or_reset(&cxlr_dax->dev, region_extent_unregister,
> region_extent);
> @@ -184,17 +197,16 @@ static bool extents_contain(struct cxl_dax_region *cxlr_dax,
> struct cxl_endpoint_decoder *cxled,
> struct range *new_range)
> {
> - struct region_extent *re = cxlr_dax->region_extent;
> + struct region_extent *re;
> struct cxled_extent *entry;
> - unsigned long index;
> -
> - if (!re)
> - return false;
> -
> - xa_for_each(&re->decoder_extents, index, entry) {
> - if (cxled == entry->cxled &&
> - range_contains(&entry->dpa_range, new_range))
> - return true;
> + unsigned long i, j;
> +
> + xa_for_each(&cxlr_dax->region_extents, i, re) {
> + xa_for_each(&re->decoder_extents, j, entry) {
> + if (cxled == entry->cxled &&
> + range_contains(&entry->dpa_range, new_range))
> + return true;
> + }
> }
> return false;
> }
> @@ -203,17 +215,16 @@ static bool extents_overlap(struct cxl_dax_region *cxlr_dax,
> struct cxl_endpoint_decoder *cxled,
> struct range *new_range)
> {
> - struct region_extent *re = cxlr_dax->region_extent;
> + struct region_extent *re;
> struct cxled_extent *entry;
> - unsigned long index;
> -
> - if (!re)
> - return false;
> -
> - xa_for_each(&re->decoder_extents, index, entry) {
> - if (cxled == entry->cxled &&
> - range_overlaps(&entry->dpa_range, new_range))
> - return true;
> + unsigned long i, j;
> +
> + xa_for_each(&cxlr_dax->region_extents, i, re) {
> + xa_for_each(&re->decoder_extents, j, entry) {
> + if (cxled == entry->cxled &&
> + range_overlaps(&entry->dpa_range, new_range))
> + return true;
> + }
> }
Second time seeing this pattern. Maybe just create a helper function?
> return false;
> }
> @@ -306,21 +317,25 @@ int cxl_rm_extent(struct cxl_memdev_state *mds, struct cxl_extent *extent)
> }
>
> cxlr_dax = cxlr->cxlr_dax;
> - reg_ext = cxlr_dax->region_extent;
> + import_uuid(&tag, extent->uuid);
> + {
> + struct region_extent *r;
> + unsigned long idx;
> +
> + reg_ext = NULL;
> + xa_for_each(&cxlr_dax->region_extents, idx, r) {
> + if (uuid_equal(&r->uuid, &tag)) {
> + reg_ext = r;
> + break;
> + }
> + }
> + }
Would prefer not have the extra scope.
DJ
> if (!reg_ext) {
> - dev_err(&cxlr->cxlr_dax->dev,
> - "no capacity has been added to the region\n");
> + dev_err(&cxlr_dax->dev,
> + "no region_extent matches tag %pU\n", &tag);
> return -ENXIO;
> }
>
> - import_uuid(&tag, extent->uuid);
> - if (!uuid_equal(&tag, ®_ext->uuid)) {
> - dev_err(&cxlr->cxlr_dax->dev,
> - "extent tag %pU doesn't match region tag %pU\n",
> - &tag, ®_ext->uuid);
> - return -EINVAL;
> - }
> -
> calc_hpa_range(cxled, cxlr_dax, &dpa_range, &hpa_range);
> if (!range_contains(®_ext->hpa_range, &hpa_range)) {
> dev_err(&cxlr_dax->dev,
> @@ -329,9 +344,7 @@ int cxl_rm_extent(struct cxl_memdev_state *mds, struct cxl_extent *extent)
> return -EINVAL;
> }
>
> - rc = cxlr_notify_extent(cxlr,
> - DCD_RELEASE_CAPACITY,
> - cxlr_dax->region_extent);
> + rc = cxlr_notify_extent(cxlr, DCD_RELEASE_CAPACITY, reg_ext);
> if (rc == -EBUSY)
> return 0;
>
> @@ -416,7 +429,7 @@ int cxl_add_extent(struct cxl_memdev_state *mds, struct cxl_extent *extent)
>
> cxlr_dax = cxlr->cxlr_dax;
> /* Cannot add to a region_extent once it's been onlined */
> - if (cxlr_dax->region_extent) {
> + if (!xa_empty(&cxlr_dax->region_extents)) {
> dev_err(&cxlr_dax->dev, "Can no longer add to region %d\n",
> cxlr->id);
> return -EINVAL;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 15065d9a1cbf2..42b3a3bbd502f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3546,6 +3546,7 @@ static void cxl_dax_region_release(struct device *dev)
> {
> struct cxl_dax_region *cxlr_dax = to_cxl_dax_region(dev);
>
> + xa_destroy(&cxlr_dax->region_extents);
> kfree(cxlr_dax);
> }
>
> @@ -3590,6 +3591,7 @@ static struct cxl_dax_region *cxl_dax_region_alloc(struct cxl_region *cxlr)
> if (!cxlr_dax)
> return ERR_PTR(-ENOMEM);
>
> + xa_init(&cxlr_dax->region_extents);
> cxlr_dax->hpa_range.start = p->res->start;
> cxlr_dax->hpa_range.end = p->res->end;
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 53d8a2f0d1272..715fa9e580cb0 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -637,7 +637,14 @@ struct cxl_dax_region {
> struct device dev;
> struct cxl_region *cxlr;
> struct range hpa_range;
> - struct region_extent *region_extent;
> + /*
> + * region_extents is keyed by an allocator-assigned u32 (see
> + * online_region_extent()), not by extent UUID. The UUID is a
> + * 128-bit pseudo-random identity carried on each region_extent;
> + * tag lookup is a linear xa_for_each walk, which is adequate at
> + * the bounded per-region tag counts this driver handles.
> + */
> + struct xarray region_extents;
> };
>
> /**