| 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: Mon, 6 May 2024 22:04:23 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: <ira.weiny@...el.com>, Dave Jiang <dave.jiang@...el.com>, Fan Ni
<fan.ni@...sung.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
"Navneet Singh" <navneet.singh@...el.com>
CC: 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>, Ira Weiny <ira.weiny@...el.com>,
<linux-btrfs@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 18/26] cxl/mem: Handle DCD add & release capacity events.
ira.weiny@ wrote:
> From: Navneet Singh <navneet.singh@...el.com>
>
> A dynamic capacity devices (DCD) send events to signal the host about
> changes in the availability of Dynamic Capacity (DC) memory. These
> events contain extents, the addition or removal of which may occur at
> any time.
>
> Adding memory is straight forward. If no region exists the extent is
> rejected. If a region does exist, a region extent is formed and
> surfaced.
>
> Removing memory requires checking if the memory is currently in use.
> Memory use tracking is added in a subsequent patch so here the memory is
> never in use and the removal occurs immediately.
>
> Most often extents will be offered to and accepted by the host in well
> defined chunks. However, part of an extent may be requested for
> release. Simplify extent tracking by signaling removal of any extent
> which overlaps the requested release range.
>
> Force removal is intended as a mechanism between the FM and the device
> and intended only when the host is unresponsive or otherwise broken.
> Purposely ignore force removal events.
>
> Process DCD extents.
>
> Recall that all devices of an interleave set must offer a corresponding
> extent for the region extent to be realized. This patch limits
> interleave to 1. Thus the 1:1 mapping between device extent and DAX
> region extent allows immediate surfacing.
>
> Signed-off-by: Navneet Singh <navneet.singh@...el.com>
> Co-developed-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
>
> ---
> Changes for v1
> [iweiny: remove all xarrays]
> [iweiny: entirely new architecture]
> ---
> drivers/cxl/core/extent.c | 4 ++
> drivers/cxl/core/mbox.c | 142 +++++++++++++++++++++++++++++++++++++++++++---
> drivers/cxl/core/region.c | 139 ++++++++++++++++++++++++++++++++++++++++-----
> drivers/cxl/cxl.h | 34 +++++++++++
> drivers/cxl/cxlmem.h | 21 +++----
> drivers/cxl/mem.c | 45 +++++++++++++++
> drivers/dax/cxl.c | 22 +++++++
> include/linux/cxl-event.h | 31 ++++++++++
> 8 files changed, 405 insertions(+), 33 deletions(-)
>
[..]
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 7635ff109578..a07d95136f0d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[..]
> @@ -1502,18 +1552,7 @@ int cxl_ed_add_one_extent(struct cxl_endpoint_decoder *cxled,
> dev_dbg(dev, "Adding DC extent DPA %#llx - %#llx\n",
> ext_dpa_range.start, ext_dpa_range.end);
>
> - /*
> - * Without interleave...
> - * HPA offset == DPA offset
> - * ... but do the math anyway
> - */
> - dpa_offset = ext_dpa_range.start - cxled->dpa_res->start;
> - hpa = cxled->cxld.hpa_range.start + dpa_offset;
> -
> - ext_hpa_range = (struct range) {
> - .start = hpa - cxlr->cxlr_dax->hpa_range.start,
> - .end = ext_hpa_range.start + range_len(&ext_dpa_range) - 1,
> - };
Please don't refactor code that just got added in the same series. Upon
seeing that this wants a common helper in this patch, go back to the
original patch and put it in a helper from the beginning.
[..]
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5379ad7f5852..156d7c9a8de5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
[..]
> @@ -891,10 +900,18 @@ bool is_cxl_region(struct device *dev);
>
> extern struct bus_type cxl_bus_type;
I skipped ahead here in the review since notification organization feels
wrong.
> +/* Driver Notifier Data */
> +struct cxl_drv_nd {
I never would have guessed that cxl_drv_nd meant cxl driver notifier
data, it might be able to be jettisoned.
> + enum dc_event event;
> + struct cxl_dc_extent *dc_extent;
> + struct region_extent *reg_ext;
> +};
> +
> struct cxl_driver {
> const char *name;
> int (*probe)(struct device *dev);
> void (*remove)(struct device *dev);
> + int (*notify)(struct device *dev, struct cxl_drv_nd *nd);
First, this feels an overly DCD specific mechanism to inflict on the core
generic 'struct cxl_driver'. Most 'struct cxl_driver' instances do not
need any 'notify' callback and 'struct cxl_drv_nd' makes this even less
relevant to the core 'struct cxl_driver' definition.
Second, it leads to 2 anonymous ->notify() callbacks wht too deep of a
stack. It feels as if the resulting code is being actively evasive.
Given that the event handling code already knows how to lookup a 'struct
cxl_region', as Alison demonstrated in her DPA->HPA series, it should be
straightforward to lookup a 'struct cxl_dax_region' without a notifying
the cxl_mem driver.
So my expectation is just enough DCD event parsing to determine when the
payload applies to given cxl_dax_region. Then define a:
struct cxl_dax_region_driver {
struct cxl_driver driver;
void (*notify)(struct cxl_dax_region *cxlr_dax, ...);
};
..to send the payload over for further processing. If a cxl_dax_region
device instance cannot be found, just drop the event record.
Powered by blists - more mailing lists