[permalink] [raw]

2024-05-04 04:13:44

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set_mode()

Ira Weiny wrote:
> Alison Schofield wrote:
> > On Sun, Mar 24, 2024 at 04:18:08PM -0700, Ira Weiny wrote:
> > > cxl_dpa_set_mode() checks the mode for validity two times, once outside
> > > of the DPA RW semaphore and again within.
> >
> > Not true.
>
> Sorry for not being clear. It does check the mode 2x but not for
> validity. I'll clarify.
>
> > It only checks mode once before the lock. It checks for
> > capacity after the lock. If it didn't check mode before the lock,
> > then unsupported modes would fall through.
>
> But we can check the mode 1 time and either check the size or fail.
>
> >
> > > The function is not in a critical path.
> >
> > Implying what here? OK to check twice (even though it wasn't)
> > or OK to expand scope of locking.
>
> Implying that checking the mode outside the lock is not required.
>
> >
> > > Prior to Dynamic Capacity the extra check was not much
> > > of an issue. The addition of DC modes increases the complexity of
> > > the check.
> > >
> > > Simplify the mode check before adding the more complex DC modes.
> > >
> >
> > The addition of the DC mode check doesn't seem complex.
>
> It is if you have to check it 2 times.
>
> >
> > Pardon my picking at the words, but if you'd like to refactor the
> > function, just say so. The final result is a bit more readable, but
> > also adding the DC mode checks without refactoring would read fine
> > also.
>
> When I added the DC mode to this function without this refactoring it was
> quite a bit more code and ugly IMO. So this cleanup helped. If I were
> not adding the DC code there would be much less reason to change this
> function.

Where did the "quite a bit more code" come from? A change that moves
unnecessary code under a lock and is larger than just incrementally
extending the status quo does not feel like a cleanup.

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 7d97790b893d..0dc886bc22c6 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -411,11 +411,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_dev_state *cxlds = cxlmd->cxlds;
struct device *dev = &cxled->cxld.dev;
- int rc;
+ int rc, dcd;

switch (mode) {
case CXL_DECODER_RAM:
case CXL_DECODER_PMEM:
+ case CXL_DECODER_DC0 ... CXL_DECODER_DC7:
break;
default:
dev_dbg(dev, "unsupported mode: %d\n", mode);
@@ -442,6 +443,11 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
rc = -ENXIO;
goto out;
}
+ dcd = dc_mode_to_region_index(mode);
+ if (resource_size(&cxlds->dc_res[dcd]) == 0) {
+ dev_dbg(dev, "no available dynamic capacity\n");
+ goto out;
+ }

cxled->mode = mode;
rc = 0;

2024-05-06 03:47:32

by Ira Weiny

[permalink] [raw]

Subject: Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set_mode()

Dan Williams wrote:
> Ira Weiny wrote:
> > Alison Schofield wrote:
> > > On Sun, Mar 24, 2024 at 04:18:08PM -0700, Ira Weiny wrote:
> > > > cxl_dpa_set_mode() checks the mode for validity two times, once outside
> > > > of the DPA RW semaphore and again within.
> > >
> > > Not true.
> >
> > Sorry for not being clear. It does check the mode 2x but not for
> > validity. I'll clarify.
> >
> > > It only checks mode once before the lock. It checks for
> > > capacity after the lock. If it didn't check mode before the lock,
> > > then unsupported modes would fall through.
> >
> > But we can check the mode 1 time and either check the size or fail.
> >
> > >
> > > > The function is not in a critical path.
> > >
> > > Implying what here? OK to check twice (even though it wasn't)
> > > or OK to expand scope of locking.
> >
> > Implying that checking the mode outside the lock is not required.
> >
> > >
> > > > Prior to Dynamic Capacity the extra check was not much
> > > > of an issue. The addition of DC modes increases the complexity of
> > > > the check.
> > > >
> > > > Simplify the mode check before adding the more complex DC modes.
> > > >
> > >
> > > The addition of the DC mode check doesn't seem complex.
> >
> > It is if you have to check it 2 times.
> >
> > >
> > > Pardon my picking at the words, but if you'd like to refactor the
> > > function, just say so. The final result is a bit more readable, but
> > > also adding the DC mode checks without refactoring would read fine
> > > also.
> >
> > When I added the DC mode to this function without this refactoring it was
> > quite a bit more code and ugly IMO. So this cleanup helped. If I were
> > not adding the DC code there would be much less reason to change this
> > function.
>
> Where did the "quite a bit more code" come from? A change that moves
> unnecessary code under a lock and is larger than just incrementally
> extending the status quo does not feel like a cleanup.

I'll drop the patch.

Ira

>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 7d97790b893d..0dc886bc22c6 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -411,11 +411,12 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct device *dev = &cxled->cxld.dev;
> - int rc;
> + int rc, dcd;
>
> switch (mode) {
> case CXL_DECODER_RAM:
> case CXL_DECODER_PMEM:
> + case CXL_DECODER_DC0 ... CXL_DECODER_DC7:
> break;
> default:
> dev_dbg(dev, "unsupported mode: %d\n", mode);
> @@ -442,6 +443,11 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> rc = -ENXIO;
> goto out;
> }
> + dcd = dc_mode_to_region_index(mode);
> + if (resource_size(&cxlds->dc_res[dcd]) == 0) {
> + dev_dbg(dev, "no available dynamic capacity\n");
> + goto out;
> + }
>
> cxled->mode = mode;
> rc = 0;

2024-05-06 04:06:25

by Ira Weiny

[permalink] [raw]

The following change is preferred to show the correct mode name.

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 35ee565e27c9..729006ca4997 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -684,7 +684,7 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)

if (size > avail) {
dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size,
- cxled->mode == CXL_DECODER_RAM ? "ram" : "pmem",
+ cxl_decoder_mode_name(cxled->mode),
&avail);
rc = -ENOSPC;
goto out;

On 25/03/2024 07:18, [email protected] wrote:
> From: Navneet Singh <[email protected]>
>
> Region mode must reflect a general dynamic capacity type which is
> associated with a specific Dynamic Capacity (DC) partitions in each
> device decoder within the region. DC partitions are also know as DC
> regions per CXL 3.1.
>
> Decoder mode reflects a specific DC partition.
>
> Define the new modes to use in subsequent patches and the helper
> functions required to make the association between these new modes.
>
> Signed-off-by: Navneet Singh <[email protected]>
> Co-developed-by: Ira Weiny <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> Changes for v1
> [iweiny: split out from: Add dynamic capacity cxl region support.]
> ---
> drivers/cxl/core/region.c | 4 ++++
> drivers/cxl/cxl.h | 23 +++++++++++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1723d17f121e..ec3b8c6948e9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1690,6 +1690,8 @@ static bool cxl_modes_compatible(enum cxl_region_mode rmode,
> return true;
> if (rmode == CXL_REGION_PMEM && dmode == CXL_DECODER_PMEM)
> return true;
> + if (rmode == CXL_REGION_DC && cxl_decoder_mode_is_dc(dmode))
> + return true;
>
> return false;
> }
> @@ -2824,6 +2826,8 @@ cxl_decoder_to_region_mode(enum cxl_decoder_mode mode)
> return CXL_REGION_RAM;
> case CXL_DECODER_PMEM:
> return CXL_REGION_PMEM;
> + case CXL_DECODER_DC0 ... CXL_DECODER_DC7:
> + return CXL_REGION_DC;
> case CXL_DECODER_MIXED:
> default:
> return CXL_REGION_MIXED;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 9a0cce1e6fca..3b8935089c0c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -365,6 +365,14 @@ enum cxl_decoder_mode {
> CXL_DECODER_NONE,
> CXL_DECODER_RAM,
> CXL_DECODER_PMEM,
> + CXL_DECODER_DC0,
> + CXL_DECODER_DC1,
> + CXL_DECODER_DC2,
> + CXL_DECODER_DC3,
> + CXL_DECODER_DC4,
> + CXL_DECODER_DC5,
> + CXL_DECODER_DC6,
> + CXL_DECODER_DC7,
> CXL_DECODER_MIXED,
> CXL_DECODER_DEAD,
> };
> @@ -375,6 +383,14 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode)
> [CXL_DECODER_NONE] = "none",
> [CXL_DECODER_RAM] = "ram",
> [CXL_DECODER_PMEM] = "pmem",
> + [CXL_DECODER_DC0] = "dc0",
> + [CXL_DECODER_DC1] = "dc1",
> + [CXL_DECODER_DC2] = "dc2",
> + [CXL_DECODER_DC3] = "dc3",
> + [CXL_DECODER_DC4] = "dc4",
> + [CXL_DECODER_DC5] = "dc5",
> + [CXL_DECODER_DC6] = "dc6",
> + [CXL_DECODER_DC7] = "dc7",
> [CXL_DECODER_MIXED] = "mixed",
> };
>
> @@ -383,10 +399,16 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode)
> return "mixed";
> }
>
> +static inline bool cxl_decoder_mode_is_dc(enum cxl_decoder_mode mode)
> +{
> + return (mode >= CXL_DECODER_DC0 && mode <= CXL_DECODER_DC7);
> +}
> +
> enum cxl_region_mode {
> CXL_REGION_NONE,
> CXL_REGION_RAM,
> CXL_REGION_PMEM,
> + CXL_REGION_DC,
> CXL_REGION_MIXED,
> };
>
> @@ -396,6 +418,7 @@ static inline const char *cxl_region_mode_name(enum cxl_region_mode mode)
> [CXL_REGION_NONE] = "none",
> [CXL_REGION_RAM] = "ram",
> [CXL_REGION_PMEM] = "pmem",
> + [CXL_REGION_DC] = "dc",
> [CXL_REGION_MIXED] = "mixed",
> };
>
>