| Message ID | d8760a4a0ca5b28be4eee27a2581ca8c2abe3e49.1642535478.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add partitioning support for CXL memdevs | expand |
On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > Users may want to change the partition layout of a memory > device using the CXL command line tool. How about: CXL devices may support both volatile and persistent memory capacity. The amount of device capacity set aside for each type is typically established at the factory, but some devices also allow for dynamic re-partitioning, add a command for this purpose. > Add a new CXL command, > 'cxl set-partition-info', that operates on a CXL memdev, or a > set of memdevs, and allows the user to change the partition > layout of the device(s). > > Synopsis: > Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>] It's unfortunate that the specification kept the word "info" in the set-partition command name, let's leave it out of this name and just call this 'cxl set-partition'. > > -v, --verbose turn on debug > -s, --volatile_size <n> > next volatile partition size in bytes Some users will come to this tool with the thought, "I want volatile capacity X", others "I want pmem capacity Y". All but the most advanced users will not understand how the volatile setting affects the pmem and vice versa, nor will they understand that capacity might be fixed and other capacity is dynamic. So how about a set of options like: -t, --type=<volatile,pmem> -s,--size=<size> ...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic capacity set to PMEM. The tool can then help the user get what they want relative to whatever frame of reference led them to read the set-partition man page. The tool can figure out the details behind the scenes, or warn them if they want something impossible like setting PMEM capacity to 1GB on a device where 2GB of PMEM is statically allocated. This also helps with future proofing in case there are other types that come along. > > The included MAN page explains how to find the partitioning > capabilities and restrictions of a CXL memory device. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > Documentation/cxl/cxl-set-partition-info.txt | 53 ++++++++++ > Documentation/cxl/meson.build | 1 + > cxl/builtin.h | 1 + > cxl/cxl.c | 1 + > cxl/memdev.c | 101 +++++++++++++++++++ > 5 files changed, 157 insertions(+) > create mode 100644 Documentation/cxl/cxl-set-partition-info.txt > > diff --git a/Documentation/cxl/cxl-set-partition-info.txt b/Documentation/cxl/cxl-set-partition-info.txt > new file mode 100644 > index 0000000..d99a1b9 > --- /dev/null > +++ b/Documentation/cxl/cxl-set-partition-info.txt > @@ -0,0 +1,53 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +cxl-set-partition-info(1) > +========================= > + > +NAME > +---- > +cxl-set-partition-info - set the partitioning between volatile and persistent capacity on a CXL memdev > + > +SYNOPSIS > +-------- > +[verse] > +'cxl set-partition-info <mem> [ [<mem1>..<memN>] [<options>]' > + > +DESCRIPTION > +----------- > +Partition the device into volatile and persistent capacity. The change > +in partitioning will become the “next” configuration, to become active > +on the next device reset. > + > +Use "cxl list -m <memdev> -I" to examine the partitioning capabilities > +of a device. A partition_alignment_bytes value of zero means there are > +no partitionable bytes available and therefore the partitions cannot be > +changed. > + > +Using this command to change the size of the persistent capacity shall > +result in the loss of data stored. > + > +OPTIONS > +------- > +<memory device(s)>:: > +include::memdev-option.txt[] > + > +-s:: > +--size=:: This is "--volatile_size" in the current patch, but that's moot with feedback to move to a --type + --size scheme. > + Size in bytes of the volatile partition requested. > + > + Size must align to the devices partition_alignment_bytes. > + Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes. This a static 256M, that can just be documented here. > + > + Size must be less than or equal to the device's partitionable bytes. > + Calculate partitionable bytes by subracting the volatile_only_bytes, s/subracting/subtracting/ ...but rather than teaching the user to input valid values, tell them the validation and translation the tool will do on their behalf with the input based on type and the details they don't need to worry about.
On Wed, Jan 26, 2022 at 05:44:54PM -0800, Dan Williams wrote: > On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote: > > > > From: Alison Schofield <alison.schofield@intel.com> > > > > Users may want to change the partition layout of a memory > > device using the CXL command line tool. > > How about: > > CXL devices may support both volatile and persistent memory capacity. > The amount of device capacity set aside for each type is typically > established at the factory, but some devices also allow for dynamic > re-partitioning, add a command for this purpose. Thanks! > > > Add a new CXL command, > > 'cxl set-partition-info', that operates on a CXL memdev, or a > > set of memdevs, and allows the user to change the partition > > layout of the device(s). > > > > Synopsis: > > Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>] > > It's unfortunate that the specification kept the word "info" in the > set-partition command name, let's leave it out of this name and just > call this 'cxl set-partition'. > Will do. > > > > -v, --verbose turn on debug > > -s, --volatile_size <n> > > next volatile partition size in bytes > > Some users will come to this tool with the thought, "I want volatile > capacity X", others "I want pmem capacity Y". All but the most > advanced users will not understand how the volatile setting affects > the pmem and vice versa, nor will they understand that capacity might > be fixed and other capacity is dynamic. I went very by the spec here, with those advance users in mind. I do think the user with limited knowledge may get frustrated by the rules. ie. like finding that their total capacity is not all partitionable capacity. I looked at ipmctl goal setting, where they do percentages, and didn't pursue - stuck with the spec. I like this below. If user wants 100% of any type, no math needed. But, anything else the user will have to specify a byte value. > So how about a set of options like: > > -t, --type=<volatile,pmem> > -s,--size=<size> > > ...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic > capacity set to PMEM. > I don't get the language "and -s is '0' for 100%", specifically, the "-s is 0". If -s is ommitted the default behavior will be to set 100% of the partitionable capacity to type. If both type and size are ommitted, 100% of partitionable capacity is set to PMEM. Humor me...are these right? set-partition mem0 100% to pmem set-partition mem0 -tpmem 100% to pmem set-partition mem0 -tvolatile -s0 100% to pmem ^^ That's what I think a -s0 does?!? set-partition mem0 -tvolatile 100% to volatile set-partition mem0 -tvolatile -s256MB 256MB to volatile Remainder to pmem set-partition mem0 -s256MB 256MB to pmem Remainder to volatile Thanks! > The tool can then help the user get what they want relative to > whatever frame of reference led them to read the set-partition man > page. The tool can figure out the details behind the scenes, or warn > them if they want something impossible like setting PMEM capacity to > 1GB on a device where 2GB of PMEM is statically allocated. > > This also helps with future proofing in case there are other types > that come along. > Got it. > > > > The included MAN page explains how to find the partitioning > > capabilities and restrictions of a CXL memory device. > > snip > > +------- > > +<memory device(s)>:: > > +include::memdev-option.txt[] > > + > > +-s:: > > +--size=:: > > This is "--volatile_size" in the current patch, but that's moot with > feedback to move to a --type + --size scheme. Got it. > > > + Size in bytes of the volatile partition requested. > > + > > + Size must align to the devices partition_alignment_bytes. > > + Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes. > > This a static 256M, that can just be documented here. > My read of the spec says partition alignment is "Expressed in multiples of 256MB", not static. Where is it defined as static? > > + > > + Size must be less than or equal to the device's partitionable bytes. > > + Calculate partitionable bytes by subracting the volatile_only_bytes, > > s/subracting/subtracting/ > > ...but rather than teaching the user to input valid values, tell them > the validation and translation the tool will do on their behalf with > the input based on type and the details they don't need to worry > about. Got it. Thanks Dan.
On Wed, Jan 26, 2022 at 9:40 PM Alison Schofield <alison.schofield@intel.com> wrote: > > On Wed, Jan 26, 2022 at 05:44:54PM -0800, Dan Williams wrote: > > On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote: > > > > > > From: Alison Schofield <alison.schofield@intel.com> > > > > > > Users may want to change the partition layout of a memory > > > device using the CXL command line tool. > > > > How about: > > > > CXL devices may support both volatile and persistent memory capacity. > > The amount of device capacity set aside for each type is typically > > established at the factory, but some devices also allow for dynamic > > re-partitioning, add a command for this purpose. > > Thanks! > > > > > Add a new CXL command, > > > 'cxl set-partition-info', that operates on a CXL memdev, or a > > > set of memdevs, and allows the user to change the partition > > > layout of the device(s). > > > > > > Synopsis: > > > Usage: cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>] > > > > It's unfortunate that the specification kept the word "info" in the > > set-partition command name, let's leave it out of this name and just > > call this 'cxl set-partition'. > > > Will do. > > > > > > > -v, --verbose turn on debug > > > -s, --volatile_size <n> > > > next volatile partition size in bytes > > > > Some users will come to this tool with the thought, "I want volatile > > capacity X", others "I want pmem capacity Y". All but the most > > advanced users will not understand how the volatile setting affects > > the pmem and vice versa, nor will they understand that capacity might > > be fixed and other capacity is dynamic. > > I went very by the spec here, with those advance users in mind. I do > think the user with limited knowledge may get frustrated by the rules. > ie. like finding that their total capacity is not all partitionable > capacity. I looked at ipmctl goal setting, where they do percentages, > and didn't pursue - stuck with the spec. > > I like this below. If user wants 100% of any type, no math needed. But, > anything else the user will have to specify a byte value. > > > So how about a set of options like: > > > > -t, --type=<volatile,pmem> > > -s,--size=<size> > > > > ...where by default -t is 'pmem' and -s is '0' for 100% of the dynamic > > capacity set to PMEM. > > > I don't get the language "and -s is '0' for 100%", specifically, the > "-s is 0". > > If -s is ommitted the default behavior will be to set 100% of the > partitionable capacity to type. Right, sorry, yes I should have been clear that "-s 0" on the command line means zero-out that capacity, but no "-s" is a default value that means 100% of the given type. I forgot that NULL can be used to detect string options not specified. > > If both type and size are ommitted, 100% of partitionable capacity is > set to PMEM. > > Humor me...are these right? > > set-partition mem0 > 100% to pmem yup. > > set-partition mem0 -tpmem > 100% to pmem I thought you need a space after short options, but apparently not. > > set-partition mem0 -tvolatile -s0 > 100% to pmem ^^ That's what I think a -s0 does?!? > Agree, looks good. > set-partition mem0 -tvolatile > 100% to volatile yup. > > set-partition mem0 -tvolatile -s256MB > 256MB to volatile > Remainder to pmem > > set-partition mem0 -s256MB > 256MB to pmem > Remainder to volatile Yup, thanks for going through the examples to clarify.
diff --git a/Documentation/cxl/cxl-set-partition-info.txt b/Documentation/cxl/cxl-set-partition-info.txt new file mode 100644 index 0000000..d99a1b9 --- /dev/null +++ b/Documentation/cxl/cxl-set-partition-info.txt @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0 + +cxl-set-partition-info(1) +========================= + +NAME +---- +cxl-set-partition-info - set the partitioning between volatile and persistent capacity on a CXL memdev + +SYNOPSIS +-------- +[verse] +'cxl set-partition-info <mem> [ [<mem1>..<memN>] [<options>]' + +DESCRIPTION +----------- +Partition the device into volatile and persistent capacity. The change +in partitioning will become the “next” configuration, to become active +on the next device reset. + +Use "cxl list -m <memdev> -I" to examine the partitioning capabilities +of a device. A partition_alignment_bytes value of zero means there are +no partitionable bytes available and therefore the partitions cannot be +changed. + +Using this command to change the size of the persistent capacity shall +result in the loss of data stored. + +OPTIONS +------- +<memory device(s)>:: +include::memdev-option.txt[] + +-s:: +--size=:: + Size in bytes of the volatile partition requested. + + Size must align to the devices partition_alignment_bytes. + Use 'cxl list -m <memdev> -I' to find partition_alignment_bytes. + + Size must be less than or equal to the device's partitionable bytes. + Calculate partitionable bytes by subracting the volatile_only_bytes, + and the persistent_only_bytes, from the total_bytes. + Use 'cxl list -m <memdev> -I' to find the above mentioned_byte values. + +-v:: + Turn on verbose debug messages in the library (if libcxl was built with + logging and debug enabled). + +SEE ALSO +-------- +linkcxl:cxl-list[1], +CXL-2.0 8.2.9.5.2 diff --git a/Documentation/cxl/meson.build b/Documentation/cxl/meson.build index 64ce13f..0108eea 100644 --- a/Documentation/cxl/meson.build +++ b/Documentation/cxl/meson.build @@ -28,6 +28,7 @@ cxl_manpages = [ 'cxl-read-labels.txt', 'cxl-write-labels.txt', 'cxl-zero-labels.txt', + 'cxl-set-partition-info.txt', ] foreach man : cxl_manpages diff --git a/cxl/builtin.h b/cxl/builtin.h index 78eca6e..7f11f28 100644 --- a/cxl/builtin.h +++ b/cxl/builtin.h @@ -10,4 +10,5 @@ int cmd_read_labels(int argc, const char **argv, struct cxl_ctx *ctx); int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx); int cmd_init_labels(int argc, const char **argv, struct cxl_ctx *ctx); int cmd_check_labels(int argc, const char **argv, struct cxl_ctx *ctx); +int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx); #endif /* _CXL_BUILTIN_H_ */ diff --git a/cxl/cxl.c b/cxl/cxl.c index 4b1661d..3153cf0 100644 --- a/cxl/cxl.c +++ b/cxl/cxl.c @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = { { "zero-labels", .c_fn = cmd_zero_labels }, { "read-labels", .c_fn = cmd_read_labels }, { "write-labels", .c_fn = cmd_write_labels }, + { "set-partition-info", .c_fn = cmd_set_partition_info }, }; int main(int argc, const char **argv) diff --git a/cxl/memdev.c b/cxl/memdev.c index d063d51..e1348c8 100644 --- a/cxl/memdev.c +++ b/cxl/memdev.c @@ -6,6 +6,7 @@ #include <unistd.h> #include <limits.h> #include <util/log.h> +#include <util/size.h> #include <cxl/libcxl.h> #include <util/parse-options.h> #include <ccan/minmax/minmax.h> @@ -24,6 +25,7 @@ static struct parameters { unsigned len; unsigned offset; bool verbose; + const char *volatile_size; } param; #define fail(fmt, ...) \ @@ -48,6 +50,10 @@ OPT_UINTEGER('s', "size", ¶m.len, "number of label bytes to operate"), \ OPT_UINTEGER('O', "offset", ¶m.offset, \ "offset into the label area to start operation") +#define SET_PARTITION_OPTIONS() \ +OPT_STRING('s', "volatile_size", ¶m.volatile_size, "volatile-size", \ + "next volatile partition size in bytes") + static const struct option read_options[] = { BASE_OPTIONS(), LABEL_OPTIONS(), @@ -68,6 +74,12 @@ static const struct option zero_options[] = { OPT_END(), }; +static const struct option set_partition_options[] = { + BASE_OPTIONS(), + SET_PARTITION_OPTIONS(), + OPT_END(), +}; + static int action_zero(struct cxl_memdev *memdev, struct action_context *actx) { size_t size; @@ -175,6 +187,80 @@ out: return rc; } +static int validate_partition(struct cxl_memdev *memdev, + unsigned long long volatile_request) +{ + unsigned long long total_cap, volatile_only, persistent_only; + unsigned long long partitionable_bytes, partition_align_bytes; + const char *devname = cxl_memdev_get_devname(memdev); + struct cxl_cmd *cmd; + int rc; + + cmd = cxl_cmd_new_identify(memdev); + if (!cmd) + return -ENXIO; + rc = cxl_cmd_submit(cmd); + if (rc < 0) + goto err; + rc = cxl_cmd_get_mbox_status(cmd); + if (rc != 0) + goto err; + + partition_align_bytes = cxl_cmd_identify_get_partition_align(cmd); + if (partition_align_bytes == 0) { + fprintf(stderr, "%s: no partitionable capacity\n", devname); + rc = -EINVAL; + goto err; + } + + total_cap = cxl_cmd_identify_get_total_bytes(cmd); + volatile_only = cxl_cmd_identify_get_volatile_only_bytes(cmd); + persistent_only = cxl_cmd_identify_get_persistent_only_bytes(cmd); + + partitionable_bytes = total_cap - volatile_only - persistent_only; + + if (volatile_request > partitionable_bytes) { + fprintf(stderr, "%s: volatile size %lld exceeds partitionable capacity %lld\n", + devname, volatile_request, partitionable_bytes); + rc = -EINVAL; + goto err; + } + if (!IS_ALIGNED(volatile_request, partition_align_bytes)) { + fprintf(stderr, "%s: volatile size %lld is not partition aligned %lld\n", + devname, volatile_request, partition_align_bytes); + rc = -EINVAL; + } +err: + cxl_cmd_unref(cmd); + return rc; +} + +static int action_set_partition(struct cxl_memdev *memdev, + struct action_context *actx) +{ + const char *devname = cxl_memdev_get_devname(memdev); + unsigned long long volatile_request; + int rc; + + volatile_request = parse_size64(param.volatile_size); + if (volatile_request == ULLONG_MAX) { + fprintf(stderr, "%s: failed to parse volatile size '%s'\n", + devname, param.volatile_size); + return -EINVAL; + } + + rc = validate_partition(memdev, volatile_request); + if (rc) + return rc; + + rc = cxl_memdev_set_partition_info(memdev, volatile_request, + !cxl_cmd_partition_info_flag_immediate()); + if (rc) + fprintf(stderr, "%s error: %s\n", devname, strerror(-rc)); + + return rc; +} + static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx, int (*action)(struct cxl_memdev *memdev, struct action_context *actx), const struct option *options, const char *usage) @@ -235,6 +321,11 @@ static int memdev_action(int argc, const char **argv, struct cxl_ctx *ctx, } } + if (action == action_set_partition && !param.volatile_size) { + usage_with_options(u, options); + return -EINVAL; + } + if (param.verbose) cxl_set_log_priority(ctx, LOG_DEBUG); @@ -323,3 +414,13 @@ int cmd_zero_labels(int argc, const char **argv, struct cxl_ctx *ctx) count > 1 ? "s" : ""); return count >= 0 ? 0 : EXIT_FAILURE; } + +int cmd_set_partition_info(int argc, const char **argv, struct cxl_ctx *ctx) +{ + int count = memdev_action(argc, argv, ctx, action_set_partition, + set_partition_options, + "cxl set-partition-info <mem0> [<mem1>..<memN>] [<options>]"); + fprintf(stderr, "set_partition %d mem%s\n", count >= 0 ? count : 0, + count > 1 ? "s" : ""); + return count >= 0 ? 0 : EXIT_FAILURE; +}