| Message ID | 20230120-vv-volatile-regions-v1-5-b42b21ee8d0b@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add support for listing and creating volatile regions | expand |
Vishal Verma wrote: > In the common case, root decoders are expected to be either pmem > capable, or volatile capable, but not necessarily both simultaneously. > If a decoder only has one of pmem or volatile capabilities, > cxl-create-region should just infer the type of the region (pmem > or ram) based on this capability. > > Maintain the default behavior of cxl-create-region to choose type=pmem, > but only as a fallback if the selected root decoder has multiple > capabilities. If it is only capable of either pmem, or ram, then infer > region type from this without requiring it to be specified explicitly. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > Documentation/cxl/cxl-create-region.txt | 3 ++- > cxl/region.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt > index ada0e52..f11a412 100644 > --- a/Documentation/cxl/cxl-create-region.txt > +++ b/Documentation/cxl/cxl-create-region.txt > @@ -75,7 +75,8 @@ include::bus-option.txt[] > > -t:: > --type=:: > - Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'. > + Specify the region type - 'pmem' or 'ram'. Default to root decoder > + capability, and if that is ambiguous, default to 'pmem'. > > -U:: > --uuid=:: > diff --git a/cxl/region.c b/cxl/region.c > index 9079b2d..1c8ccc7 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder, > return 0; > } > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) > +{ > + int num_cap = 0; > + > + /* if param.type was explicitly specified, nothing to do here */ > + if (param.type) > + return; > + > + /* > + * if the root decoder only has one type of capability, default > + * to that mode for the region. > + */ > + if (cxl_decoder_is_pmem_capable(p->root_decoder)) > + num_cap++; > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > + num_cap++; > + > + if (num_cap == 1) { > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > + p->mode = CXL_DECODER_MODE_RAM; > + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) > + p->mode = CXL_DECODER_MODE_PMEM; > + } I feel like the default for p->mode should be moved here from parse_create_options(). But I'm not sure what the flows might be like in that case. That means p->mode would default to NONE until here. That would make the man page behavior and this function match up nicely for future maintenance. But I don't think this is wrong. So: Reviewed-by: Ira Weiny <ira.weiny@intel.com> > +} > + > static int create_region_validate_config(struct cxl_ctx *ctx, > struct parsed_params *p) > { > @@ -481,6 +506,8 @@ found: > return -ENXIO; > } > > + set_type_from_decoder(ctx, p); > + > rc = validate_decoder(p->root_decoder, p); > if (rc) > return rc; > > -- > 2.39.1 > >
Vishal Verma wrote: > In the common case, root decoders are expected to be either pmem > capable, or volatile capable, but not necessarily both simultaneously. > If a decoder only has one of pmem or volatile capabilities, > cxl-create-region should just infer the type of the region (pmem > or ram) based on this capability. > > Maintain the default behavior of cxl-create-region to choose type=pmem, > but only as a fallback if the selected root decoder has multiple > capabilities. If it is only capable of either pmem, or ram, then infer > region type from this without requiring it to be specified explicitly. > > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > Documentation/cxl/cxl-create-region.txt | 3 ++- > cxl/region.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt > index ada0e52..f11a412 100644 > --- a/Documentation/cxl/cxl-create-region.txt > +++ b/Documentation/cxl/cxl-create-region.txt > @@ -75,7 +75,8 @@ include::bus-option.txt[] > > -t:: > --type=:: > - Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'. > + Specify the region type - 'pmem' or 'ram'. Default to root decoder > + capability, and if that is ambiguous, default to 'pmem'. > > -U:: > --uuid=:: > diff --git a/cxl/region.c b/cxl/region.c > index 9079b2d..1c8ccc7 100644 > --- a/cxl/region.c > +++ b/cxl/region.c > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder, > return 0; > } > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) > +{ > + int num_cap = 0; > + > + /* if param.type was explicitly specified, nothing to do here */ > + if (param.type) > + return; > + > + /* > + * if the root decoder only has one type of capability, default > + * to that mode for the region. > + */ > + if (cxl_decoder_is_pmem_capable(p->root_decoder)) > + num_cap++; > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > + num_cap++; > + > + if (num_cap == 1) { > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > + p->mode = CXL_DECODER_MODE_RAM; > + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) > + p->mode = CXL_DECODER_MODE_PMEM; > + } Is @num_cap needed? I.e. if this just does: if (cxl_decoder_is_volatile_capable(p->root_decoder)) p->mode = CXL_DECODER_MODE_RAM; if (cxl_decoder_is_pmem_capable(p->root_decoder)) p->mode = CXL_DECODER_MODE_PMEM; ...then it matches the changelog of defaulting to pmem if both types are set, and otherwise the single capability dominates.
On Tue, 2023-02-07 at 20:07 -0800, Ira Weiny wrote: > Vishal Verma wrote: > > > > diff --git a/cxl/region.c b/cxl/region.c > > index 9079b2d..1c8ccc7 100644 > > --- a/cxl/region.c > > +++ b/cxl/region.c > > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder, > > return 0; > > } > > > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) > > +{ > > + int num_cap = 0; > > + > > + /* if param.type was explicitly specified, nothing to do here */ > > + if (param.type) > > + return; > > + > > + /* > > + * if the root decoder only has one type of capability, default > > + * to that mode for the region. > > + */ > > + if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > + num_cap++; > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > + num_cap++; > > + > > + if (num_cap == 1) { > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > + p->mode = CXL_DECODER_MODE_RAM; > > + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > + p->mode = CXL_DECODER_MODE_PMEM; > > + } > > I feel like the default for p->mode should be moved here from > parse_create_options(). But I'm not sure what the flows might be like in > that case. That means p->mode would default to NONE until here. > > That would make the man page behavior and this function match up nicely > for future maintenance. Hm, do they not match now? I can see the appeal in collecting the default mode setup in one place, but in my mind the early checks / defaults in parse_create_options() are a simple, initial pass for canned defaults, and conflicting option checks. Things like set_type_from_decoder() are more of a 'second pass' thing where we may do more involved porcelain type decision making based on the full topology. > > But I don't think this is wrong. So: > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Tue, 2023-02-07 at 21:55 -0800, Dan Williams wrote: > Vishal Verma wrote: > > <..> > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) > > +{ > > + int num_cap = 0; > > + > > + /* if param.type was explicitly specified, nothing to do here */ > > + if (param.type) > > + return; > > + > > + /* > > + * if the root decoder only has one type of capability, default > > + * to that mode for the region. > > + */ > > + if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > + num_cap++; > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > + num_cap++; > > + > > + if (num_cap == 1) { > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > + p->mode = CXL_DECODER_MODE_RAM; > > + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > + p->mode = CXL_DECODER_MODE_PMEM; > > + } > > Is @num_cap needed? I.e. if this just does: > > if (cxl_decoder_is_volatile_capable(p->root_decoder)) > p->mode = CXL_DECODER_MODE_RAM; > if (cxl_decoder_is_pmem_capable(p->root_decoder)) > p->mode = CXL_DECODER_MODE_PMEM; > > ...then it matches the changelog of defaulting to pmem if both types are > set, and otherwise the single capability dominates. Oh true, that's clever! I hope it isn't too clever for future-me when coming across this and wondering what is happening, but for now I like it, so I'll run with it :)
Verma, Vishal L wrote: > On Tue, 2023-02-07 at 20:07 -0800, Ira Weiny wrote: > > Vishal Verma wrote: > > > > > > diff --git a/cxl/region.c b/cxl/region.c > > > index 9079b2d..1c8ccc7 100644 > > > --- a/cxl/region.c > > > +++ b/cxl/region.c > > > @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder, > > > return 0; > > > } > > > > > > +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) > > > +{ > > > + int num_cap = 0; > > > + > > > + /* if param.type was explicitly specified, nothing to do here */ > > > + if (param.type) > > > + return; > > > + > > > + /* > > > + * if the root decoder only has one type of capability, default > > > + * to that mode for the region. > > > + */ > > > + if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > > + num_cap++; > > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > > + num_cap++; > > > + > > > + if (num_cap == 1) { > > > + if (cxl_decoder_is_volatile_capable(p->root_decoder)) > > > + p->mode = CXL_DECODER_MODE_RAM; > > > + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) > > > + p->mode = CXL_DECODER_MODE_PMEM; > > > + } > > > > I feel like the default for p->mode should be moved here from > > parse_create_options(). But I'm not sure what the flows might be like in > > that case. That means p->mode would default to NONE until here. > > > > That would make the man page behavior and this function match up nicely > > for future maintenance. > > Hm, do they not match now? As I said it is correct. > I can see the appeal in collecting the > default mode setup in one place, but in my mind the early checks / > defaults in parse_create_options() are a simple, initial pass for > canned defaults, and conflicting option checks. Things like > set_type_from_decoder() are more of a 'second pass' thing where we may > do more involved porcelain type decision making based on the full > topology. Yea I can see it that way too. > > > > > But I don't think this is wrong. So: > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> My tag stands. Ira
diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt index ada0e52..f11a412 100644 --- a/Documentation/cxl/cxl-create-region.txt +++ b/Documentation/cxl/cxl-create-region.txt @@ -75,7 +75,8 @@ include::bus-option.txt[] -t:: --type=:: - Specify the region type - 'pmem' or 'ram'. Defaults to 'pmem'. + Specify the region type - 'pmem' or 'ram'. Default to root decoder + capability, and if that is ambiguous, default to 'pmem'. -U:: --uuid=:: diff --git a/cxl/region.c b/cxl/region.c index 9079b2d..1c8ccc7 100644 --- a/cxl/region.c +++ b/cxl/region.c @@ -448,6 +448,31 @@ static int validate_decoder(struct cxl_decoder *decoder, return 0; } +static void set_type_from_decoder(struct cxl_ctx *ctx, struct parsed_params *p) +{ + int num_cap = 0; + + /* if param.type was explicitly specified, nothing to do here */ + if (param.type) + return; + + /* + * if the root decoder only has one type of capability, default + * to that mode for the region. + */ + if (cxl_decoder_is_pmem_capable(p->root_decoder)) + num_cap++; + if (cxl_decoder_is_volatile_capable(p->root_decoder)) + num_cap++; + + if (num_cap == 1) { + if (cxl_decoder_is_volatile_capable(p->root_decoder)) + p->mode = CXL_DECODER_MODE_RAM; + else if (cxl_decoder_is_pmem_capable(p->root_decoder)) + p->mode = CXL_DECODER_MODE_PMEM; + } +} + static int create_region_validate_config(struct cxl_ctx *ctx, struct parsed_params *p) { @@ -481,6 +506,8 @@ found: return -ENXIO; } + set_type_from_decoder(ctx, p); + rc = validate_decoder(p->root_decoder, p); if (rc) return rc;
In the common case, root decoders are expected to be either pmem capable, or volatile capable, but not necessarily both simultaneously. If a decoder only has one of pmem or volatile capabilities, cxl-create-region should just infer the type of the region (pmem or ram) based on this capability. Maintain the default behavior of cxl-create-region to choose type=pmem, but only as a fallback if the selected root decoder has multiple capabilities. If it is only capable of either pmem, or ram, then infer region type from this without requiring it to be specified explicitly. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- Documentation/cxl/cxl-create-region.txt | 3 ++- cxl/region.c | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-)