| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 4 Apr 2024 18:36:56 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ira Weiny <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Navneet
Singh" <navneet.singh@...el.com>, Dan Williams <dan.j.williams@...el.com>,
Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>,
<linux-btrfs@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 22/26] dax/region: Support DAX device creation on sparse
DAX regions
On Sun, 24 Mar 2024 16:18:25 -0700
Ira Weiny <ira.weiny@...el.com> wrote:
> Previous patches introduced a new sparse DAX region type. This region
> type may have 0 or more bytes of backing memory.
>
> DAX devices already have the ability to reference sparse ranges of a DAX
> region. Leverage the range support of DAX devices to track memory
> across a sparse set of region extents.
>
> Requests for extent removal can be received from the device at any time.
> But the host is not obliged to release that memory until it is finished
> with it. Introduce a use count to track how many DAX devices are using
> an extent. If that extent is in use reject the removal of the extent.
>
> Leverage the region RW semaphore to protect the extent data as any
> changes to the use of the extent require DAX device, DAX region, and
> extent stability during those operations.
>
> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
Comments are minor. I'm not 100% confident on this yet, but
that's more a case of I need to look at the end result of the whole
series. Fairly happy though so...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>
> ---
> Changes for v3
> [iweiny: simplify the extent objects]
> [iweiny: refactor based on the new extent objects created]
> [iweiny: remove xarray]
> [iweiny: use lock/invalidate/cnt rather than kref]
> ---
> drivers/cxl/core/extent.c | 8 ++
> drivers/cxl/core/region.c | 6 +-
> drivers/cxl/cxl.h | 1 +
> drivers/dax/bus.c | 191 +++++++++++++++++++++++++++++++++++++++-------
> drivers/dax/bus.h | 3 +-
> drivers/dax/cxl.c | 55 ++++++++++++-
> drivers/dax/dax-private.h | 23 ++++++
> drivers/dax/hmem/hmem.c | 2 +-
> drivers/dax/pmem.c | 2 +-
> 9 files changed, 258 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 56dddaceeccb..70a559763e8c 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -236,11 +236,32 @@ int dax_region_add_extent(struct dax_region *dax_region, struct device *ext_dev,
> if (rc)
> return rc;
>
> - return devm_add_action_or_reset(ext_dev, dax_region_release_extent,
> + /* Assume the devm action will be configured without error */
> + dev_set_drvdata(ext_dev, dax_ext);
> + rc = devm_add_action_or_reset(ext_dev, dax_region_release_extent,
> no_free_ptr(dax_ext));
Indent needs tweaking.
> + if (rc)
> + dev_set_drvdata(ext_dev, NULL);
> + return rc;
> }
> EXPORT_SYMBOL_GPL(dax_region_add_extent);
> @@ -507,15 +553,26 @@ EXPORT_SYMBOL_GPL(kill_dev_dax);
> static void trim_dev_dax_range(struct dev_dax *dev_dax)
> {
> int i = dev_dax->nr_range - 1;
> - struct range *range = &dev_dax->ranges[i].range;
> + struct dev_dax_range *dev_range = &dev_dax->ranges[i];
> + struct range *range = &dev_range->range;
> struct dax_region *dax_region = dev_dax->region;
> + struct resource *res = &dax_region->res;
>
> WARN_ON_ONCE(!rwsem_is_locked(&dax_region_rwsem));
> dev_dbg(&dev_dax->dev, "delete range[%d]: %#llx:%#llx\n", i,
> (unsigned long long)range->start,
> (unsigned long long)range->end);
>
> - __release_region(&dax_region->res, range->start, range_len(range));
> + if (dev_range->dax_ext) {
> + res = dev_range->dax_ext->res;
> + dev_dbg(&dev_dax->dev, "Trim sparse extent %pr\n", res);
> + }
> +
> + __release_region(res, range->start, range_len(range));
> +
> + if (dev_range->dax_ext)
May be worth considering splitting this core bit into two
functions for dev_range->dax_ext and not. The overriding of res
is not giving nice readable code.
> + dev_range->dax_ext->use_cnt--;
> +
> if (--dev_dax->nr_range == 0) {
> kfree(dev_dax->ranges);
> dev_dax->ranges = NULL;
>
> /**
> - * dev_dax_resize_static - Expand the device into the unused portion of the
> - * region. This may involve adjusting the end of an existing resource, or
> - * allocating a new resource.
> + * __dev_dax_resize - Expand the device into the unused portion of the region.
> + * This may involve adjusting the end of an existing resource, or allocating a
> + * new resource.
> *
> * @parent: parent resource to allocate this range in
> * @dev_dax: DAX device to be expanded
> * @to_alloc: amount of space to alloc; must be <= space available in @parent
> + * @dax_ext: if sparse; the extent containing parent
If not, what? NULL, but maybe docs should make that explicit.
> *
> * Return the amount of space allocated or -ERRNO on failure
> */
> +static ssize_t dev_dax_resize_sparse(struct dax_region *dax_region,
> + struct dev_dax *dev_dax,
> + resource_size_t to_alloc)
> +{
> + struct dax_extent *dax_ext;
> + resource_size_t extent_max;
> + struct device *ext_dev;
> + ssize_t alloc;
> +
> + ext_dev = dax_region->sparse_ops->find_ext(dax_region, &extent_max,
> + dax_ext_match_avail_size);
> + if (!ext_dev)
> + return -ENOSPC;
> +
> + dax_ext = dev_get_drvdata(ext_dev);
> + if (!dax_ext)
> + return -ENOSPC;
> +
> + to_alloc = min(extent_max, to_alloc);
> + alloc = __dev_dax_resize(dax_ext->res, dev_dax, to_alloc, dax_ext);
> + if (alloc < 0)
> + dax_ext->use_cnt--;
Maybe define a put_dax_ext() / get_dax_ext() given this is operating somewhat like that
in that find_ext takes a reference and that is dropped on error.
> + return alloc;
> +}
> diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> index 83ee45aff69a..3cb95e5988ae 100644
> --- a/drivers/dax/cxl.c
> +++ b/drivers/dax/cxl.c
> +/**
> + * find_ext - Match Extent callback
> + * @dax_region: region to search
> + * @size_avail: the available size if an extent is found
> + * @match_fn: match function
> + *
> + * Callback to itterate through the child devices of the DAX region calling
Spell check. Iterate
> + * match_fn only on those devices which are extents.
> + *
> + * If a match is found match_fn is responsible for locking or reference
> + * counting dax_ext as needed.
> + */
> +static struct device *find_ext(struct dax_region *dax_region,
> + resource_size_t *size_avail,
> + match_cb match_fn)
> +{
> + struct match_data md = {
> + .match_fn = match_fn,
> + .size_avail = size_avail,
> + };
> + struct device *ext_dev;
> +
> + ext_dev = device_find_child(dax_region->dev, &md, cxl_dax_match_ext);
> +
Trivial but I'd drop this blank line to closely group the check with the find.
> + if (!ext_dev)
> + return NULL;
> +
> + /* caller must hold a count on extent data */
> + put_device(ext_dev);
> + return ext_dev;
> +}
Powered by blists - more mailing lists