Re: [PATCH 17/26] dax/region: Create extent resources on DAX region driver load

Dan Williams
2024-05-07T02:31:38Z

ira.weiny@ wrote:
> From: Navneet Singh <navneet.singh@intel.com>
>
> DAX regions mapping dynamic capacity partitions introduce a requirement
> for the memory backing the region to come and go as required. This
> results in a DAX region with sparse areas of memory backing. To track
> the sparseness of the region, DAX extent objects need to track
> sub-resource information as a new layer between the DAX region resource
> and DAX device range resources.
>
> Recall that DCD extents may be accepted when a region is first created.
> Extend this support on region driver load. Scan existing extents and
> create DAX extent resources as a first step to DAX extent realization.
>
> The lifetime of a DAX extent is tricky to manage because the extent life
> may end in one of two ways. First, the device may request the extent be
> released. Second, the region may release the extent when it is
> destroyed without hardware involvement. Support extent release without
> hardware involvement first. Subsequent patches will provide for
> hardware to request extent removal.
>
> Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---
> Changes for v1
> [iweiny: remove xarrays]
> [iweiny: remove as much of extra reference stuff as possible]
> [iweiny: Move extent resource handling to core DAX code]
> ---
> drivers/dax/bus.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/dax/cxl.c | 43 ++++++++++++++++++++++++++++++++++--
> drivers/dax/dax-private.h | 12 +++++++++++
> 3 files changed, 108 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 903566aff5eb..4d5ed7ab6537 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -186,6 +186,61 @@ static bool is_sparse(struct dax_region *dax_region)
> return (dax_region->res.flags & IORESOURCE_DAX_SPARSE_CAP) != 0;
> }
>
> +static int dax_region_add_resource(struct dax_region *dax_region,
> + struct dax_extent *dax_ext,
> + resource_size_t start,
> + resource_size_t length)
> +{
> + struct resource *ext_res;
> +
> + dev_dbg(dax_region->dev, "DAX region resource %pr\n", &dax_region->res);
> + ext_res = __request_region(&dax_region->res, start, length, "extent", 0);
> + if (!ext_res) {
> + dev_err(dax_region->dev, "Failed to add region s:%pa l:%pa\n",
> + &start, &length);
> + return -ENOSPC;
> + }
> +
> + dax_ext->region = dax_region;
> + dax_ext->res = ext_res;
> + dev_dbg(dax_region->dev, "Extent add resource %pr\n", ext_res);

dax_ext is never used, it feels like this these helpers are in the wrong
patch. Like consumer side of dax_ext infrastructure lands *before* the
producer side.

Because the justification for this producer-side patch is after the case
for 'struct dax_extent' has been made.

> +int dax_region_add_extent(struct dax_region *dax_region, struct device *ext_dev,
> + resource_size_t start, resource_size_t length)
> +{
> + int rc;
> +
> + struct dax_extent *dax_ext __free(kfree) = kzalloc(sizeof(*dax_ext),
> + GFP_KERNEL);
> + if (!dax_ext)
> + return -ENOMEM;
> +
> + guard(rwsem_write)(&dax_region_rwsem);
> + rc = dax_region_add_resource(dax_region, dax_ext, start, length);
> + if (rc)
> + return rc;
> +
> + return devm_add_action_or_reset(ext_dev, dax_region_release_extent,
> + no_free_ptr(dax_ext));

This looks like an awkward rewrite of __devm_request_region(), but
likely that is because dax_ext is vestigial in this patch.

> +static void cxl_dax_region_add_extents(struct cxl_dax_region *cxlr_dax,
> + struct dax_region *dax_region)
> +{
> + dev_dbg(&cxlr_dax->dev, "Adding extents\n");
> + device_for_each_child(&cxlr_dax->dev, dax_region, cxl_dax_region_add_extent);

Per the comment on the last patch to move extent device creation to
cxl_dax_region_probe() that can get rid of looping over those devices
another time.