From: Dan Williams <dan.j.williams(a)intel.com&gt; Re-indent util_daxctl_region_filter() to match expectations. Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- util/filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/filter.c b/util/filter.c index af72793929e2..7d0159f8cef6 100644 --- a/util/filter.c +++ b/util/filter.c @@ -344,7 +344,7 @@ struct daxctl_region *util_daxctl_region_filter(struct daxctl_region *region, return region; if ((sscanf(ident, "%d", &region_id) == 1 - || sscanf(ident, "region%d", &region_id) == 1) + || sscanf(ident, "region%d", &region_id) == 1) && daxctl_region_get_id(region) == region_id) return region; -- 1.8.3.1

Add the ability to change the size of an inactive dax device. Uses of this includes adjusting existing devices or enterily freeing the region device to make the space available. $ daxctl disable-device dax0.0 disabled 1 device $ daxctl reconfigure-device -s 0 dax0.0 reconfigured 1 device $ daxctl reconfigure-device -s 4G dax0.0 reconfigured 1 device @size (-s) and @mode (-m) are mutually exclusive as the latter relates to assigning memory to System-RAM through kmem as opposed to reconfiguring dynamic dax devices. Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- Documentation/daxctl/daxctl-reconfigure-device.txt | 16 +++++++++++ daxctl/device.c | 32 ++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt index cb28fed24e52..8caae436faae 100644 --- a/Documentation/daxctl/daxctl-reconfigure-device.txt +++ b/Documentation/daxctl/daxctl-reconfigure-device.txt @@ -75,6 +75,14 @@ daxctl_dev_get_target_node() or 'daxctl list') # numactl --cpunodebind=0-1 --membind=2 -- some-service --opt1 --opt2 ---- +* Change the size of a dax device +---- +# daxctl reconfigure-device dax0.1 -s 16G +reconfigured 1 device +# daxctl reconfigure-device dax0.1 -s 0 +reconfigured 1 device +---- + DESCRIPTION ----------- @@ -120,6 +128,14 @@ OPTIONS more /dev/daxX.Y devices, where X is the region id and Y is the device instance id. +-s:: +--size=:: + For regions that support dax device creation, change the device size + in bytes. This option supports the suffixes "k" or "K" for KiB, "m" or + "M" for MiB, "g" or "G" for GiB and "t" or "T" for TiB. + + The size must be a multiple of the region alignment. + -m:: --mode=:: Specify the mode to which the dax device(s) should be reconfigured. diff --git a/daxctl/device.c b/daxctl/device.c index 705f1f8ff7f6..033e098eafe0 100644 --- a/daxctl/device.c +++ b/daxctl/device.c @@ -9,6 +9,7 @@ #include <sys/stat.h> #include <sys/types.h> #include <sys/sysmacros.h> +#include <util/size.h> #include <util/json.h> #include <util/filter.h> #include <json-c/json.h> @@ -20,6 +21,7 @@ static struct { const char *dev; const char *mode; const char *region; + const char *size; bool no_online; bool no_movable; bool force; @@ -34,6 +36,7 @@ enum dev_mode { }; static enum dev_mode reconfig_mode = DAXCTL_DEV_MODE_UNKNOWN; +static long long size = -1; static unsigned long flags; enum memory_zone { @@ -60,12 +63,16 @@ OPT_BOOLEAN('N', "no-online", &param.no_online, \ OPT_BOOLEAN('f', "force", &param.force, \ "attempt to offline memory sections before reconfiguration") +#define CREATE_OPTIONS() \ +OPT_STRING('s', "size", &param.size, "size", "size to switch the device to") + #define ZONE_OPTIONS() \ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \ "online memory in ZONE_NORMAL") static const struct option reconfig_options[] = { BASE_OPTIONS(), + CREATE_OPTIONS(), RECONFIG_OPTIONS(), ZONE_OPTIONS(), OPT_END(), @@ -90,6 +97,7 @@ static const char *parse_device_options(int argc, const char **argv, usage, NULL }; + unsigned long long units = 1; int i, rc = 0; argc = parse_options(argc, argv, options, u, 0); @@ -135,12 +143,14 @@ static const char *parse_device_options(int argc, const char **argv, /* Handle action-specific options */ switch (action) { case ACTION_RECONFIG: - if (!param.mode) { - fprintf(stderr, "error: a 'mode' option is required\n"); + if (!param.size && !param.mode) { + fprintf(stderr, "error: a 'mode' or 'size' option is required\n"); usage_with_options(u, reconfig_options); rc = -EINVAL; } - if (strcmp(param.mode, "system-ram") == 0) { + if (param.size) { + size = __parse_size64(param.size, &units); + } else if (strcmp(param.mode, "system-ram") == 0) { reconfig_mode = DAXCTL_DEV_MODE_RAM; if (param.no_movable) mem_zone = MEM_ZONE_NORMAL; @@ -309,6 +319,17 @@ static int dev_offline_memory(struct daxctl_dev *dev) return 0; } +static int dev_resize(struct daxctl_dev *dev, unsigned long long val) +{ + int rc; + + rc = daxctl_dev_set_size(dev, val); + if (rc < 0) + return rc; + + return 0; +} + static int disable_devdax_device(struct daxctl_dev *dev) { struct daxctl_memory *mem = daxctl_dev_get_memory(dev); @@ -418,6 +439,11 @@ static int do_reconfig(struct daxctl_dev *dev, enum dev_mode mode, struct json_object *jdev; int rc = 0; + if (size >= 0) { + rc = dev_resize(dev, size); + return rc; + } + switch (mode) { case DAXCTL_DEV_MODE_RAM: rc = reconfig_mode_system_ram(dev); -- 1.8.3.1

Add a 'enable-device' command, required prior to reconfiguration of the dax device. Mimics the same functionality as seen in ndctl-enable-namespace. Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- Documentation/daxctl/Makefile.am | 3 +- Documentation/daxctl/daxctl-enable-device.txt | 59 +++++++++++++++++++++++++++ daxctl/builtin.h | 1 + daxctl/daxctl.c | 1 + daxctl/device.c | 40 ++++++++++++++++++ 5 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 Documentation/daxctl/daxctl-enable-device.txt diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am index 1f070771cd95..e43d34183142 100644 --- a/Documentation/daxctl/Makefile.am +++ b/Documentation/daxctl/Makefile.am @@ -32,7 +32,8 @@ man1_MANS = \ daxctl-reconfigure-device.1 \ daxctl-online-memory.1 \ daxctl-offline-memory.1 \ - daxctl-disable-device.1 + daxctl-disable-device.1 \ + daxctl-enable-device.1 EXTRA_DIST = $(man1_MANS) diff --git a/Documentation/daxctl/daxctl-enable-device.txt b/Documentation/daxctl/daxctl-enable-device.txt new file mode 100644 index 000000000000..6410d92cafe9 --- /dev/null +++ b/Documentation/daxctl/daxctl-enable-device.txt @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0 + +daxctl-enable-device(1) +======================= + +NAME +---- +daxctl-enable-device - Enable a devdax device + +SYNOPSIS +-------- +[verse] +'daxctl enable-device' <dax0.0> [<dax1.0>...<daxY.Z>] [<options>] + +EXAMPLES +-------- + +* Enables dax0.1 +---- +# daxctl enable-device dax0.1 +enabled 1 device +---- + +* Enables all devices in region id 0 +---- +# daxctl enable-device -r 0 all +enabled 3 devices +---- + +DESCRIPTION +----------- + +Enables a dax device in 'devdax' mode. + +OPTIONS +------- +-r:: +--region=:: + Restrict the operation to devices belonging to the specified region(s). + A device-dax region is a contiguous range of memory that hosts one or + more /dev/daxX.Y devices, where X is the region id and Y is the device + instance id. + +-u:: +--human:: + By default the command will output machine-friendly raw-integer + data. Instead, with this flag, numbers representing storage size + will be formatted as human readable strings with units, other + fields are converted to hexadecimal strings. + +-v:: +--verbose:: + Emit more debug messages + +include::../copyright.txt[] + +SEE ALSO +-------- +linkdaxctl:daxctl-list[1],daxctl-reconfigure-device[1],daxctl-create-device[1] diff --git a/daxctl/builtin.h b/daxctl/builtin.h index c9848953bbd8..8f344f86ad20 100644 --- a/daxctl/builtin.h +++ b/daxctl/builtin.h @@ -8,6 +8,7 @@ int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_disable_device(int argc, const char **argv, struct daxctl_ctx *ctx); +int cmd_enable_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_online_memory(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_offline_memory(int argc, const char **argv, struct daxctl_ctx *ctx); #endif /* _DAXCTL_BUILTIN_H_ */ diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c index 1707a9ff0791..a4699b3780bd 100644 --- a/daxctl/daxctl.c +++ b/daxctl/daxctl.c @@ -75,6 +75,7 @@ static struct cmd_struct commands[] = { { "online-memory", .d_fn = cmd_online_memory }, { "offline-memory", .d_fn = cmd_offline_memory }, { "disable-device", .d_fn = cmd_disable_device }, + { "enable-device", .d_fn = cmd_enable_device }, }; int main(int argc, const char **argv) diff --git a/daxctl/device.c b/daxctl/device.c index 20df2b844774..05a9247ecfde 100644 --- a/daxctl/device.c +++ b/daxctl/device.c @@ -50,6 +50,7 @@ enum device_action { ACTION_ONLINE, ACTION_OFFLINE, ACTION_DISABLE, + ACTION_ENABLE, }; #define BASE_OPTIONS() \ @@ -95,6 +96,11 @@ static const struct option disable_options[] = { OPT_END(), }; +static const struct option enable_options[] = { + BASE_OPTIONS(), + OPT_END(), +}; + static const char *parse_device_options(int argc, const char **argv, enum device_action action, const struct option *options, const char *usage, struct daxctl_ctx *ctx) @@ -125,6 +131,9 @@ static const char *parse_device_options(int argc, const char **argv, case ACTION_DISABLE: action_string = "disable"; break; + case ACTION_ENABLE: + action_string = "enable"; + break; default: action_string = "<>"; break; @@ -178,6 +187,7 @@ static const char *parse_device_options(int argc, const char **argv, /* fall through */ case ACTION_OFFLINE: case ACTION_DISABLE: + case ACTION_ENABLE: /* nothing special */ break; } @@ -521,6 +531,14 @@ static int do_xble(struct daxctl_dev *dev, enum device_action action) } switch (action) { + case ACTION_ENABLE: + rc = daxctl_dev_enable_devdax(dev); + if (rc) { + fprintf(stderr, "%s: enable failed: %s\n", + daxctl_dev_get_devname(dev), strerror(-rc)); + return rc; + } + break; case ACTION_DISABLE: rc = daxctl_dev_disable(dev); if (rc) { @@ -570,6 +588,11 @@ static int do_xaction_device(const char *device, enum device_action action, if (rc == 0) (*processed)++; break; + case ACTION_ENABLE: + rc = do_xble(dev, action); + if (rc == 0) + (*processed)++; + break; case ACTION_DISABLE: rc = do_xble(dev, action); if (rc == 0) @@ -627,6 +650,23 @@ int cmd_disable_device(int argc, const char **argv, struct daxctl_ctx *ctx) return rc; } +int cmd_enable_device(int argc, const char **argv, struct daxctl_ctx *ctx) +{ + char *usage = "daxctl enable-device <device>"; + const char *device = parse_device_options(argc, argv, ACTION_DISABLE, + enable_options, usage, ctx); + int processed, rc; + + rc = do_xaction_device(device, ACTION_ENABLE, ctx, &processed); + if (rc < 0) + fprintf(stderr, "error enabling device: %s\n", + strerror(-rc)); + + fprintf(stderr, "enabled %d device%s\n", processed, + processed == 1 ? "" : "s"); + return rc; +} + int cmd_online_memory(int argc, const char **argv, struct daxctl_ctx *ctx) { char *usage = "daxctl online-memory <device> [<options>]"; -- 1.8.3.1

Add a 'create-device' command which creates a devdax region. So far the only new option is size, which when omitted uses the entire available space. Example usage: $ daxctl reconfigure-device -s 0 dax0.0 reconfigured 1 device $ daxctl create-device -s 4G -r 0 [ { "chardev":"dax0.1", "size":4294967296, "target_node":0, "mode":"devdax" } ] created 1 device Or using the whole available space like default behaviour or ndctl-create-namespace: $ daxctl create-device [ { "chardev":"dax0.1", "size":120259084288, "target_node":0, "mode":"devdax" } ] created 1 device Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- Documentation/daxctl/Makefile.am | 3 +- Documentation/daxctl/daxctl-create-device.txt | 105 ++++++++++++++++++++++++ daxctl/builtin.h | 1 + daxctl/daxctl.c | 1 + daxctl/device.c | 111 +++++++++++++++++++++++++- 5 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 Documentation/daxctl/daxctl-create-device.txt diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am index e43d34183142..27e201dfc254 100644 --- a/Documentation/daxctl/Makefile.am +++ b/Documentation/daxctl/Makefile.am @@ -33,7 +33,8 @@ man1_MANS = \ daxctl-online-memory.1 \ daxctl-offline-memory.1 \ daxctl-disable-device.1 \ - daxctl-enable-device.1 + daxctl-enable-device.1 \ + daxctl-create-device.1 EXTRA_DIST = $(man1_MANS) diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt new file mode 100644 index 000000000000..648d2541f833 --- /dev/null +++ b/Documentation/daxctl/daxctl-create-device.txt @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-2.0 + +daxctl-create-device(1) +======================= + +NAME +---- +daxctl-create-device - Create a devdax device + +SYNOPSIS +-------- +[verse] +'daxctl create-device' [<options>] + +EXAMPLES +-------- + +* Creates dax0.1 with 4G of size +---- +# daxctl create-device -s 4G +[ + { + "chardev":"dax0.1", + "size":4294967296, + "target_node":0, + "mode":"devdax" + } +] +---- + +* Creates devices with fully available size on all regions +---- +# daxctl create-device -u +[ + { + "chardev":"dax0.1", + "size":"15.63 GiB (16.78 GB)", + "target_node":0, + "mode":"devdax" + }, + { + "chardev":"dax1.1", + "size":"15.63 GiB (16.78 GB)", + "target_node":1, + "mode":"devdax" + } +] +---- + +* Creates dax0.1 with fully available size on region id 0 +---- +# daxctl create-device -r 0 -u +{ + "chardev":"dax0.1", + "size":"15.63 GiB (16.78 GB)", + "target_node":0, + "mode":"devdax" +} +---- + +DESCRIPTION +----------- + +Creates dax device in 'devdax' mode in dynamic regions. The resultant can also +be convereted to the 'system-ram' mode which arranges for the dax range to be +hot-plugged into the system as regular memory. + +'daxctl create-device' expects that the BIOS or kernel defines a range in the +EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's +100% capacity is reserved for applications. + +OPTIONS +------- +-r:: +--region=:: + Restrict the operation to devices belonging to the specified region(s). + A device-dax region is a contiguous range of memory that hosts one or + more /dev/daxX.Y devices, where X is the region id and Y is the device + instance id. + +-s:: +--size=:: + For regions that support dax device cretion, set the device size + in bytes. Otherwise it defaults to the maximum size specified by + region. This option supports the suffixes "k" or "K" for KiB, "m" or + "M" for MiB, "g" or "G" for GiB and "t" or "T" for TiB. + + The size must be a multiple of the region alignment. + +-u:: +--human:: + By default the command will output machine-friendly raw-integer + data. Instead, with this flag, numbers representing storage size + will be formatted as human readable strings with units, other + fields are converted to hexadecimal strings. + +-v:: +--verbose:: + Emit more debug messages + +include::../copyright.txt[] + +SEE ALSO +-------- +linkdaxctl:daxctl-list[1],daxctl-reconfigure-device[1],daxctl-destroy-device[1] diff --git a/daxctl/builtin.h b/daxctl/builtin.h index 8f344f86ad20..19b33933b91b 100644 --- a/daxctl/builtin.h +++ b/daxctl/builtin.h @@ -6,6 +6,7 @@ struct daxctl_ctx; int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx); +int cmd_create_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_disable_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_enable_device(int argc, const char **argv, struct daxctl_ctx *ctx); diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c index a4699b3780bd..1f315168c513 100644 --- a/daxctl/daxctl.c +++ b/daxctl/daxctl.c @@ -71,6 +71,7 @@ static struct cmd_struct commands[] = { { "list", .d_fn = cmd_list }, { "help", .d_fn = cmd_help }, { "migrate-device-model", .d_fn = cmd_migrate }, + { "create-device", .d_fn = cmd_create_device }, { "reconfigure-device", .d_fn = cmd_reconfig_device }, { "online-memory", .d_fn = cmd_online_memory }, { "offline-memory", .d_fn = cmd_offline_memory }, diff --git a/daxctl/device.c b/daxctl/device.c index 05a9247ecfde..c038abba8063 100644 --- a/daxctl/device.c +++ b/daxctl/device.c @@ -49,6 +49,7 @@ enum device_action { ACTION_RECONFIG, ACTION_ONLINE, ACTION_OFFLINE, + ACTION_CREATE, ACTION_DISABLE, ACTION_ENABLE, }; @@ -72,6 +73,14 @@ OPT_STRING('s', "size", &param.size, "size", "size to switch the device to") OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \ "online memory in ZONE_NORMAL") +static const struct option create_options[] = { + BASE_OPTIONS(), + CREATE_OPTIONS(), + RECONFIG_OPTIONS(), + ZONE_OPTIONS(), + OPT_END(), +}; + static const struct option reconfig_options[] = { BASE_OPTIONS(), CREATE_OPTIONS(), @@ -115,7 +124,8 @@ static const char *parse_device_options(int argc, const char **argv, argc = parse_options(argc, argv, options, u, 0); /* Handle action-agnostic non-option arguments */ - if (argc == 0) { + if (argc == 0 && + action != ACTION_CREATE) { char *action_string; switch (action) { @@ -181,6 +191,10 @@ static const char *parse_device_options(int argc, const char **argv, } } break; + case ACTION_CREATE: + if (param.size) + size = __parse_size64(param.size, &units); + /* fall through */ case ACTION_ONLINE: if (param.no_movable) mem_zone = MEM_ZONE_NORMAL; @@ -452,6 +466,47 @@ static int reconfig_mode_devdax(struct daxctl_dev *dev) return 0; } +static int do_create(struct daxctl_region *region, long long val, + struct json_object **jdevs) +{ + struct json_object *jdev; + struct daxctl_dev *dev; + int rc = 0; + + if (daxctl_region_create_dev(region)) + return -ENOSPC; + + dev = daxctl_region_get_dev_seed(region); + if (!dev) + return -ENOSPC; + + if (val == -1) + val = daxctl_region_get_available_size(region); + + if (val <= 0) + return -ENOSPC; + + rc = daxctl_dev_set_size(dev, val); + if (rc < 0) + return rc; + + rc = daxctl_dev_enable_devdax(dev); + if (rc) { + fprintf(stderr, "%s: enable failed: %s\n", + daxctl_dev_get_devname(dev), strerror(-rc)); + return rc; + } + + *jdevs = json_object_new_array(); + if (*jdevs) { + jdev = util_daxctl_dev_to_json(dev, flags); + if (jdev) + json_object_array_add(*jdevs, jdev); + } + + return 0; +} + static int do_reconfig(struct daxctl_dev *dev, enum dev_mode mode, struct json_object **jdevs) { @@ -554,6 +609,42 @@ static int do_xble(struct daxctl_dev *dev, enum device_action action) return rc; } +static int do_xaction_region(enum device_action action, + struct daxctl_ctx *ctx, int *processed) +{ + struct json_object *jdevs = NULL; + struct daxctl_region *region; + int rc = -ENXIO; + + *processed = 0; + + daxctl_region_foreach(ctx, region) { + if (!util_daxctl_region_filter(region, param.region)) + continue; + + switch (action) { + case ACTION_CREATE: + rc = do_create(region, size, &jdevs); + if (rc == 0) + (*processed)++; + break; + default: + rc = -EINVAL; + break; + } + } + + /* + * jdevs is the containing json array for all devices we are reporting + * on. It therefore needs to be outside the region/device iterators, + * and passed in to the do_<action> functions to add their objects to + */ + if (jdevs) + util_display_json_array(stdout, jdevs, flags); + + return rc; +} + static int do_xaction_device(const char *device, enum device_action action, struct daxctl_ctx *ctx, int *processed) { @@ -616,6 +707,24 @@ static int do_xaction_device(const char *device, enum device_action action, return rc; } +int cmd_create_device(int argc, const char **argv, struct daxctl_ctx *ctx) +{ + char *usage = "daxctl create-device [<options>]"; + int processed, rc; + + parse_device_options(argc, argv, ACTION_CREATE, + create_options, usage, ctx); + + rc = do_xaction_region(ACTION_CREATE, ctx, &processed); + if (rc < 0) + fprintf(stderr, "error creating devices: %s\n", + strerror(-rc)); + + fprintf(stderr, "created %d device%s\n", processed, + processed == 1 ? "" : "s"); + return rc; +} + int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx) { char *usage = "daxctl reconfigure-device <device> [<options>]"; -- 1.8.3.1

Add a 'destroy-device' command which destroys a dax device. Like namespaces, the device needs to be disabled in order to be destroyed. Example usage: $ daxctl disable-device dax0.1 disabled 1 device $ daxctl destroy-device dax0.1 destroyed 1 device Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- Documentation/daxctl/Makefile.am | 3 +- Documentation/daxctl/daxctl-destroy-device.txt | 63 ++++++++++++++++++++++++ daxctl/builtin.h | 1 + daxctl/daxctl.c | 1 + daxctl/device.c | 66 ++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 Documentation/daxctl/daxctl-destroy-device.txt diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am index 27e201dfc254..2b3f92ca5b96 100644 --- a/Documentation/daxctl/Makefile.am +++ b/Documentation/daxctl/Makefile.am @@ -34,7 +34,8 @@ man1_MANS = \ daxctl-offline-memory.1 \ daxctl-disable-device.1 \ daxctl-enable-device.1 \ - daxctl-create-device.1 + daxctl-create-device.1 \ + daxctl-destroy-device.1 EXTRA_DIST = $(man1_MANS) diff --git a/Documentation/daxctl/daxctl-destroy-device.txt b/Documentation/daxctl/daxctl-destroy-device.txt new file mode 100644 index 000000000000..1c91cb2fab75 --- /dev/null +++ b/Documentation/daxctl/daxctl-destroy-device.txt @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0 + +daxctl-destroy-device(1) +======================== + +NAME +---- +daxctl-destroy-device - Destroy a devdax device + +SYNOPSIS +-------- +[verse] +'daxctl destroy-device' <dax0.0> [<dax1.0>...<daxY.Z>] [<options>] + +EXAMPLES +-------- + +* Destroys dax0.1 +---- +# daxctl disable-device dax0.1 +disabled 1 device +# daxctl destroy-device dax0.1 +destroyed 1 device +---- + +* Destroys all devices in region id 0 +---- +# daxctl disable-device -r 0 all +disabled 3 devices +# daxctl destroy-device -r 0 all +destroyed 2 devices +---- + +DESCRIPTION +----------- + +Destroys a dax device in 'devdax' mode. + +OPTIONS +------- +-r:: +--region=:: + Restrict the operation to devices belonging to the specified region(s). + A device-dax region is a contiguous range of memory that hosts one or + more /dev/daxX.Y devices, where X is the region id and Y is the device + instance id. + +-u:: +--human:: + By default the command will output machine-friendly raw-integer + data. Instead, with this flag, numbers representing storage size + will be formatted as human readable strings with units, other + fields are converted to hexadecimal strings. + +-v:: +--verbose:: + Emit more debug messages + +include::../copyright.txt[] + +SEE ALSO +-------- +linkdaxctl:daxctl-list[1],daxctl-reconfigure-device[1],daxctl-create-device[1] diff --git a/daxctl/builtin.h b/daxctl/builtin.h index 19b33933b91b..29ba63ca17aa 100644 --- a/daxctl/builtin.h +++ b/daxctl/builtin.h @@ -7,6 +7,7 @@ struct daxctl_ctx; int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_create_device(int argc, const char **argv, struct daxctl_ctx *ctx); +int cmd_destroy_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_disable_device(int argc, const char **argv, struct daxctl_ctx *ctx); int cmd_enable_device(int argc, const char **argv, struct daxctl_ctx *ctx); diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c index 1f315168c513..bd5539900391 100644 --- a/daxctl/daxctl.c +++ b/daxctl/daxctl.c @@ -72,6 +72,7 @@ static struct cmd_struct commands[] = { { "help", .d_fn = cmd_help }, { "migrate-device-model", .d_fn = cmd_migrate }, { "create-device", .d_fn = cmd_create_device }, + { "destroy-device", .d_fn = cmd_destroy_device }, { "reconfigure-device", .d_fn = cmd_reconfig_device }, { "online-memory", .d_fn = cmd_online_memory }, { "offline-memory", .d_fn = cmd_offline_memory }, diff --git a/daxctl/device.c b/daxctl/device.c index c038abba8063..05293d6c38ee 100644 --- a/daxctl/device.c +++ b/daxctl/device.c @@ -52,6 +52,7 @@ enum device_action { ACTION_CREATE, ACTION_DISABLE, ACTION_ENABLE, + ACTION_DESTROY, }; #define BASE_OPTIONS() \ @@ -69,6 +70,10 @@ OPT_BOOLEAN('f', "force", &param.force, \ #define CREATE_OPTIONS() \ OPT_STRING('s', "size", &param.size, "size", "size to switch the device to") +#define DESTROY_OPTIONS() \ +OPT_BOOLEAN('f', "force", &param.force, \ + "attempt to disable before destroying device") + #define ZONE_OPTIONS() \ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \ "online memory in ZONE_NORMAL") @@ -110,6 +115,12 @@ static const struct option enable_options[] = { OPT_END(), }; +static const struct option destroy_options[] = { + BASE_OPTIONS(), + DESTROY_OPTIONS(), + OPT_END(), +}; + static const char *parse_device_options(int argc, const char **argv, enum device_action action, const struct option *options, const char *usage, struct daxctl_ctx *ctx) @@ -144,6 +155,9 @@ static const char *parse_device_options(int argc, const char **argv, case ACTION_ENABLE: action_string = "enable"; break; + case ACTION_DESTROY: + action_string = "destroy"; + break; default: action_string = "<>"; break; @@ -199,6 +213,7 @@ static const char *parse_device_options(int argc, const char **argv, if (param.no_movable) mem_zone = MEM_ZONE_NORMAL; /* fall through */ + case ACTION_DESTROY: case ACTION_OFFLINE: case ACTION_DISABLE: case ACTION_ENABLE: @@ -364,6 +379,35 @@ static int dev_resize(struct daxctl_dev *dev, unsigned long long val) return 0; } +static int dev_destroy(struct daxctl_dev *dev) +{ + const char *devname = daxctl_dev_get_devname(dev); + int rc; + + if (daxctl_dev_is_enabled(dev) && !param.force) { + fprintf(stderr, "%s is active, specify --force for deletion\n", + devname); + return -ENXIO; + } else { + rc = daxctl_dev_disable(dev); + if (rc) { + fprintf(stderr, "%s: disable failed: %s\n", + daxctl_dev_get_devname(dev), strerror(-rc)); + return rc; + } + } + + rc = daxctl_dev_set_size(dev, 0); + if (rc < 0) + return rc; + + rc = daxctl_region_destroy_dev(daxctl_dev_get_region(dev), dev); + if (rc < 0) + return rc; + + return 0; +} + static int disable_devdax_device(struct daxctl_dev *dev) { struct daxctl_memory *mem = daxctl_dev_get_memory(dev); @@ -689,6 +733,11 @@ static int do_xaction_device(const char *device, enum device_action action, if (rc == 0) (*processed)++; break; + case ACTION_DESTROY: + rc = dev_destroy(dev); + if (rc == 0) + (*processed)++; + break; default: rc = -EINVAL; break; @@ -725,6 +774,23 @@ int cmd_create_device(int argc, const char **argv, struct daxctl_ctx *ctx) return rc; } +int cmd_destroy_device(int argc, const char **argv, struct daxctl_ctx *ctx) +{ + char *usage = "daxctl destroy-device <device> [<options>]"; + const char *device = parse_device_options(argc, argv, ACTION_DESTROY, + destroy_options, usage, ctx); + int processed, rc; + + rc = do_xaction_device(device, ACTION_DESTROY, ctx, &processed); + if (rc < 0) + fprintf(stderr, "error destroying devices: %s\n", + strerror(-rc)); + + fprintf(stderr, "destroyed %d device%s\n", processed, + processed == 1 ? "" : "s"); + return rc; +} + int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx) { char *usage = "daxctl reconfigure-device <device> [<options>]"; -- 1.8.3.1

On 7/13/20 5:08 PM, Joao Martins wrote: Following your suggestion[0], I added a couple more validations to this test suite, covering the mappings. So on top of this patch I added the following snip below the scissors mark. Mainly, I check that the size calculated by mappingNNNN is the same as advertised by the sysfs size attribute, thus looping through all the mappings. Perhaps it would be enough to have just such validation in setup_dev() to catch the bug in [0]. But I went ahead and also validated the test cases where a certain amount of mappings are meant to be created. My only worry is the last piece in daxctl_test_adjust() where we might be tying too much on how a kernel version picks space from the region; should this logic change in an unforeseeable future (e.g. allowing space at the beginning to be adjusted). Otherwise, if this no concern, let me know and I can resend a v3 with the adjustment below. ----->8------ Subject: Validate @size versus mappingX sizes [0] https://lore.kernel.org/linux-nvdimm/CAPcyv4hFS7JS9s7cUY=2Ru2kUTRsesxwX1P... --- test/daxctl-create.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh index 0d35112b4119..8dbc00fc574f 100755 --- a/test/daxctl-create.sh +++ b/test/daxctl-create.sh @@ -46,8 +46,16 @@ find_testdev() setup_dev() { + local rc=1 + local nmaps=0 test -n "$testdev" + nmaps=$(daxctl_get_nr_mappings "$testdev") + if [[ $nmaps == 0 ]]; then + printf "Device is idle" + exit "$rc" + fi + "$DAXCTL" disable-device "$testdev" "$DAXCTL" reconfigure-device -s 0 "$testdev" available=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') @@ -110,6 +118,47 @@ daxctl_get_mode() "$DAXCTL" list -d "$1" | jq -er '.[].mode' } +daxctl_get_size_by_mapping() +{ + local size=0 + local _start=0 + local _end=0 + + _start=$(cat $1/start) + _end=$(cat $1/end) + ((size=size + _end - _start + 1)) + echo $size +} + +daxctl_get_nr_mappings() +{ + local i=0 + local _size=0 + local devsize=0 + local path="" + + path=$(readlink -f /sys/bus/dax/devices/"$1"/) + until ! [ -d $path/mapping$i ] + do + _size=$(daxctl_get_size_by_mapping "$path/mapping$i") + if [[ $msize == 0 ]]; then + i=0 + break + fi + ((devsize=devsize + _size)) + ((i=i + 1)) + done + + # Return number of mappings if the sizes between size field + # and the one computed by mappingNNN are the same + _size=$("$DAXCTL" list -d $1 | jq -er '.[0].size | .//""') + if [[ ! $_size == $devsize ]]; then + echo 0 + else + echo $i + fi +} + daxctl_test_multi() { local daxdev @@ -139,6 +188,7 @@ daxctl_test_multi() new_size=$(expr $size \* 2) daxdev_4=$("$DAXCTL" create-device -r 0 -s $new_size | jq -er '.[].chardev') test -n $daxdev_4 + test $(daxctl_get_nr_mappings $daxdev_4) -eq 2 fail_if_available @@ -173,6 +223,9 @@ daxctl_test_multi_reconfig() "$DAXCTL" reconfigure-device -s $i "$daxdev_1" done + test $(daxctl_get_nr_mappings $testdev) -eq $((ncfgs / 2)) + test $(daxctl_get_nr_mappings $daxdev_1) -eq $((ncfgs / 2)) + fail_if_available "$DAXCTL" disable-device "$daxdev_1" && "$DAXCTL" destroy-device "$daxdev_1" @@ -191,7 +244,8 @@ daxctl_test_adjust() start=$(expr $size + $size) for i in $(seq 1 1 $ncfgs) do - daxdev=$("$DAXCTL" create-device -r 0 -s $size) + daxdev=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') + test $(daxctl_get_nr_mappings $daxdev) -eq 1 done daxdev=$(daxctl_get_dev "dax0.1") @@ -202,10 +256,17 @@ daxctl_test_adjust() daxdev=$(daxctl_get_dev "dax0.2") "$DAXCTL" disable-device "$daxdev" "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + # Allocates space at the beginning: expect two mappings as + # as don't adjust the mappingX region. This is because we + # preserve the relative page_offset of existing allocations + test $(daxctl_get_nr_mappings $daxdev) -eq 2 daxdev=$(daxctl_get_dev "dax0.3") "$DAXCTL" disable-device "$daxdev" "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + # Adjusts space at the end, expect one mapping because we are + # able to extend existing region range. + test $(daxctl_get_nr_mappings $daxdev) -eq 1 fail_if_available @@ -232,6 +293,7 @@ daxctl_test1() daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') test -n "$daxdev" + test $(daxctl_get_nr_mappings $daxdev) -eq 1 fail_if_available "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev"

On Thu, 2020-12-10 at 15:01 +0000, Joao Martins wrote: Hi Joao, Thanks for the patience on these, I've gone through the patches in preparation for the next release, and they all look mostly fine. I had a few minor fixups - to the documentation and the test (fixup module name, and shellcheck complaints). I've appended a diff below of all the fixups I added. I've also included the patch below for the mapping size validation. I think the concern for future kernel layout changes is valid, but if/when that happens, we can always come back and relax or adjust the test as needed. So for now, I think having a pickier test should be better than not having one. My fixups: --- Documentation/daxctl/daxctl-create-device.txt | 18 +++----------- Documentation/daxctl/daxctl-destroy-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-disable-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-enable-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-reconfigure-device.txt | 19 ++++----------- Documentation/daxctl/human-option.txt | 8 +++++++ Documentation/daxctl/region-option.txt | 8 +++++++ Documentation/daxctl/verbose-option.txt | 5 ++++ util/filter.c | 2 +- test/daxctl-create.sh | 76 ++++++++++++++++++++++++++++++------------------------------ 10 files changed, 82 insertions(+), 120 deletions(-) --- diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt index 648d254..70029ab 100644 --- a/Documentation/daxctl/daxctl-create-device.txt +++ b/Documentation/daxctl/daxctl-create-device.txt @@ -71,12 +71,7 @@ EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -87,16 +82,9 @@ OPTIONS The size must be a multiple of the region alignment. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. +include::human-option.txt[] --v:: ---verbose:: - Emit more debug messages +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-destroy-device.txt b/Documentation/daxctl/daxctl-destroy-device.txt index 1c91cb2..a63ab0c 100644 --- a/Documentation/daxctl/daxctl-destroy-device.txt +++ b/Documentation/daxctl/daxctl-destroy-device.txt @@ -38,23 +38,11 @@ Destroys a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-disable-device.txt b/Documentation/daxctl/daxctl-disable-device.txt index 383aeeb..ee9f6e8 100644 --- a/Documentation/daxctl/daxctl-disable-device.txt +++ b/Documentation/daxctl/daxctl-disable-device.txt @@ -33,23 +33,11 @@ Disables a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-enable-device.txt b/Documentation/daxctl/daxctl-enable-device.txt index 6410d92..24cdcf3 100644 --- a/Documentation/daxctl/daxctl-enable-device.txt +++ b/Documentation/daxctl/daxctl-enable-device.txt @@ -34,23 +34,11 @@ Enables a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt index 8caae43..9a11ff5 100644 --- a/Documentation/daxctl/daxctl-reconfigure-device.txt +++ b/Documentation/daxctl/daxctl-reconfigure-device.txt @@ -121,12 +121,7 @@ refrain from then onlining it. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -161,16 +156,10 @@ include::movable-options.txt[] to offline the memory on the NUMA node associated with the dax device before converting it back to "devdax" mode. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. --v:: ---verbose:: - Emit more debug messages +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/human-option.txt b/Documentation/daxctl/human-option.txt new file mode 100644 index 0000000..2f4de7a --- /dev/null +++ b/Documentation/daxctl/human-option.txt @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 + +-u:: +--human:: + By default the command will output machine-friendly raw-integer + data. Instead, with this flag, numbers representing storage size + will be formatted as human readable strings with units, other + fields are converted to hexadecimal strings. diff --git a/Documentation/daxctl/region-option.txt b/Documentation/daxctl/region-option.txt new file mode 100644 index 0000000..a824e22 --- /dev/null +++ b/Documentation/daxctl/region-option.txt @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 + +-r:: +--region=:: + Restrict the operation to devices belonging to the specified region(s). + A device-dax region is a contiguous range of memory that hosts one or + more /dev/daxX.Y devices, where X is the region id and Y is the device + instance id. diff --git a/Documentation/daxctl/verbose-option.txt b/Documentation/daxctl/verbose-option.txt new file mode 100644 index 0000000..cb62c8e --- /dev/null +++ b/Documentation/daxctl/verbose-option.txt @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +-v:: +--verbose:: + Emit more debug messages diff --git a/util/filter.c b/util/filter.c index 7c8debb..8c78f32 100644 --- a/util/filter.c +++ b/util/filter.c @@ -342,7 +342,7 @@ struct daxctl_region *util_daxctl_region_filter(struct daxctl_region *region, return region; if ((sscanf(ident, "%d", &region_id) == 1 - || sscanf(ident, "region%d", &region_id) == 1) + || sscanf(ident, "region%d", &region_id) == 1) && daxctl_region_get_id(region) == region_id) return region; diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh index 8dbc00f..a4fbe06 100755 --- a/test/daxctl-create.sh +++ b/test/daxctl-create.sh @@ -20,8 +20,8 @@ find_testdev() # The hmem driver is needed to change the device mode, only # kernels >= v5.6 might have it available. Skip if not. - if ! modinfo hmem; then - # check if hmem is builtin + if ! modinfo dax_hmem; then + # check if dax_hmem is builtin if [ ! -d "/sys/module/device_hmem" ]; then printf "Unable to find hmem module\n" exit $rc @@ -66,7 +66,7 @@ reset_dev() test -n "$testdev" "$DAXCTL" disable-device "$testdev" - "$DAXCTL" reconfigure-device -s $available "$testdev" + "$DAXCTL" reconfigure-device -s "$available" "$testdev" "$DAXCTL" enable-device "$testdev" } @@ -76,7 +76,7 @@ reset() "$DAXCTL" disable-device -r 0 all "$DAXCTL" destroy-device -r 0 all - "$DAXCTL" reconfigure-device -s $available "$testdev" + "$DAXCTL" reconfigure-device -s "$available" "$testdev" } clear_dev() @@ -91,8 +91,8 @@ test_pass() # Available size _available_size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') - if [[ ! $_available_size == $available ]]; then - printf "Unexpected available size $_available_size != $available\n" + if [[ ! $_available_size == "$available" ]]; then + echo "Unexpected available size $_available_size != $available" exit "$rc" fi } @@ -103,7 +103,7 @@ fail_if_available() _size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') if [[ $_size ]]; then - printf "Unexpected available size $_size\n" + echo "Unexpected available size $_size" exit "$rc" fi } @@ -124,8 +124,8 @@ daxctl_get_size_by_mapping() local _start=0 local _end=0 - _start=$(cat $1/start) - _end=$(cat $1/end) + _start=$(cat "$1"/start) + _end=$(cat "$1"/end) ((size=size + _end - _start + 1)) echo $size } @@ -138,10 +138,10 @@ daxctl_get_nr_mappings() local path="" path=$(readlink -f /sys/bus/dax/devices/"$1"/) - until ! [ -d $path/mapping$i ] + until ! [ -d "$path/mapping$i" ] do _size=$(daxctl_get_size_by_mapping "$path/mapping$i") - if [[ $msize == 0 ]]; then + if [[ $_size == 0 ]]; then i=0 break fi @@ -151,8 +151,8 @@ daxctl_get_nr_mappings() # Return number of mappings if the sizes between size field # and the one computed by mappingNNN are the same - _size=$("$DAXCTL" list -d $1 | jq -er '.[0].size | .//""') - if [[ ! $_size == $devsize ]]; then + _size=$("$DAXCTL" list -d "$1" | jq -er '.[0].size | .//""') + if [[ ! $_size == "$devsize" ]]; then echo 0 else echo $i @@ -163,7 +163,7 @@ daxctl_test_multi() { local daxdev - size=$(expr $available / 4) + size=$((available / 4)) if [[ $2 ]]; then "$DAXCTL" disable-device "$testdev" @@ -171,24 +171,24 @@ daxctl_test_multi() fi daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_1 + test -n "$daxdev_1" daxdev_2=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_2 + test -n "$daxdev_2" if [[ ! $2 ]]; then daxdev_3=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_3 + test -n "$daxdev_3" fi # Hole - "$DAXCTL" disable-device $1 && "$DAXCTL" destroy-device $1 + "$DAXCTL" disable-device "$1" && "$DAXCTL" destroy-device "$1" # Pick space in the created hole and at the end - new_size=$(expr $size \* 2) - daxdev_4=$("$DAXCTL" create-device -r 0 -s $new_size | jq -er '.[].chardev') - test -n $daxdev_4 - test $(daxctl_get_nr_mappings $daxdev_4) -eq 2 + new_size=$((size * 2)) + daxdev_4=$("$DAXCTL" create-device -r 0 -s "$new_size" | jq -er '.[].chardev') + test -n "$daxdev_4" + test "$(daxctl_get_nr_mappings "$daxdev_4")" -eq 2 fail_if_available @@ -201,7 +201,7 @@ daxctl_test_multi_reconfig() local ncfgs=$1 local daxdev - size=$(expr $available / $ncfgs) + size=$((available / ncfgs)) test -n "$testdev" @@ -212,19 +212,19 @@ daxctl_test_multi_reconfig() daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') "$DAXCTL" disable-device "$daxdev_1" - start=$(expr $size + $size) - max=$(expr $ncfgs / 2 \* $size) + start=$((size + size)) + max=$((size * ncfgs / 2)) for i in $(seq $start $size $max) do "$DAXCTL" disable-device "$testdev" - "$DAXCTL" reconfigure-device -s $i "$testdev" + "$DAXCTL" reconfigure-device -s "$i" "$testdev" "$DAXCTL" disable-device "$daxdev_1" - "$DAXCTL" reconfigure-device -s $i "$daxdev_1" + "$DAXCTL" reconfigure-device -s "$i" "$daxdev_1" done - test $(daxctl_get_nr_mappings $testdev) -eq $((ncfgs / 2)) - test $(daxctl_get_nr_mappings $daxdev_1) -eq $((ncfgs / 2)) + test "$(daxctl_get_nr_mappings "$testdev")" -eq $((ncfgs / 2)) + test "$(daxctl_get_nr_mappings "$daxdev_1")" -eq $((ncfgs / 2)) fail_if_available @@ -237,15 +237,15 @@ daxctl_test_adjust() local ncfgs=4 local daxdev - size=$(expr $available / $ncfgs) + size=$((available / ncfgs)) test -n "$testdev" - start=$(expr $size + $size) + start=$((size + size)) for i in $(seq 1 1 $ncfgs) do - daxdev=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + daxdev=$("$DAXCTL" create-device -r 0 -s "$size" | jq -er '.[].chardev') + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 done daxdev=$(daxctl_get_dev "dax0.1") @@ -255,18 +255,18 @@ daxctl_test_adjust() daxdev=$(daxctl_get_dev "dax0.2") "$DAXCTL" disable-device "$daxdev" - "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + "$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev" # Allocates space at the beginning: expect two mappings as # as don't adjust the mappingX region. This is because we # preserve the relative page_offset of existing allocations - test $(daxctl_get_nr_mappings $daxdev) -eq 2 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 2 daxdev=$(daxctl_get_dev "dax0.3") "$DAXCTL" disable-device "$daxdev" - "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + "$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev" # Adjusts space at the end, expect one mapping because we are # able to extend existing region range. - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 fail_if_available @@ -293,7 +293,7 @@ daxctl_test1() daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') test -n "$daxdev" - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 fail_if_available "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev"

On Thu, 2020-12-10 at 15:01 +0000, Joao Martins wrote: Hi Joao, Thanks for the patience on these, I've gone through the patches in preparation for the next release, and they all look mostly fine. I had a few minor fixups - to the documentation and the test (fixup module name, and shellcheck complaints). I've appended a diff below of all the fixups I added. I've also included the patch below for the mapping size validation. I think the concern for future kernel layout changes is valid, but if/when that happens, we can always come back and relax or adjust the test as needed. So for now, I think having a pickier test should be better than not having one. My fixups: --- Documentation/daxctl/daxctl-create-device.txt | 18 +++----------- Documentation/daxctl/daxctl-destroy-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-disable-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-enable-device.txt | 22 ++++-------------- Documentation/daxctl/daxctl-reconfigure-device.txt | 19 ++++----------- Documentation/daxctl/human-option.txt | 8 +++++++ Documentation/daxctl/region-option.txt | 8 +++++++ Documentation/daxctl/verbose-option.txt | 5 ++++ util/filter.c | 2 +- test/daxctl-create.sh | 76 ++++++++++++++++++++++++++++++------------------------------ 10 files changed, 82 insertions(+), 120 deletions(-) --- diff --git a/Documentation/daxctl/daxctl-create-device.txt b/Documentation/daxctl/daxctl-create-device.txt index 648d254..70029ab 100644 --- a/Documentation/daxctl/daxctl-create-device.txt +++ b/Documentation/daxctl/daxctl-create-device.txt @@ -71,12 +71,7 @@ EFI memory map with EFI_MEMORY_SP. The resultant ranges mean that it's OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -87,16 +82,9 @@ OPTIONS The size must be a multiple of the region alignment. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. +include::human-option.txt[] --v:: ---verbose:: - Emit more debug messages +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-destroy-device.txt b/Documentation/daxctl/daxctl-destroy-device.txt index 1c91cb2..a63ab0c 100644 --- a/Documentation/daxctl/daxctl-destroy-device.txt +++ b/Documentation/daxctl/daxctl-destroy-device.txt @@ -38,23 +38,11 @@ Destroys a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-disable-device.txt b/Documentation/daxctl/daxctl-disable-device.txt index 383aeeb..ee9f6e8 100644 --- a/Documentation/daxctl/daxctl-disable-device.txt +++ b/Documentation/daxctl/daxctl-disable-device.txt @@ -33,23 +33,11 @@ Disables a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-enable-device.txt b/Documentation/daxctl/daxctl-enable-device.txt index 6410d92..24cdcf3 100644 --- a/Documentation/daxctl/daxctl-enable-device.txt +++ b/Documentation/daxctl/daxctl-enable-device.txt @@ -34,23 +34,11 @@ Enables a dax device in 'devdax' mode. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. - --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. - --v:: ---verbose:: - Emit more debug messages +include::region-option.txt[] + +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt index 8caae43..9a11ff5 100644 --- a/Documentation/daxctl/daxctl-reconfigure-device.txt +++ b/Documentation/daxctl/daxctl-reconfigure-device.txt @@ -121,12 +121,7 @@ refrain from then onlining it. OPTIONS ------- --r:: ---region=:: - Restrict the operation to devices belonging to the specified region(s). - A device-dax region is a contiguous range of memory that hosts one or - more /dev/daxX.Y devices, where X is the region id and Y is the device - instance id. +include::region-option.txt[] -s:: --size=:: @@ -161,16 +156,10 @@ include::movable-options.txt[] to offline the memory on the NUMA node associated with the dax device before converting it back to "devdax" mode. --u:: ---human:: - By default the command will output machine-friendly raw-integer - data. Instead, with this flag, numbers representing storage size - will be formatted as human readable strings with units, other - fields are converted to hexadecimal strings. --v:: ---verbose:: - Emit more debug messages +include::human-option.txt[] + +include::verbose-option.txt[] include::../copyright.txt[] diff --git a/Documentation/daxctl/human-option.txt b/Documentation/daxctl/human-option.txt new file mode 100644 index 0000000..2f4de7a --- /dev/null +++ b/Documentation/daxctl/human-option.txt @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 + +-u:: +--human:: + By default the command will output machine-friendly raw-integer + data. Instead, with this flag, numbers representing storage size + will be formatted as human readable strings with units, other + fields are converted to hexadecimal strings. diff --git a/Documentation/daxctl/region-option.txt b/Documentation/daxctl/region-option.txt new file mode 100644 index 0000000..a824e22 --- /dev/null +++ b/Documentation/daxctl/region-option.txt @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 + +-r:: +--region=:: + Restrict the operation to devices belonging to the specified region(s). + A device-dax region is a contiguous range of memory that hosts one or + more /dev/daxX.Y devices, where X is the region id and Y is the device + instance id. diff --git a/Documentation/daxctl/verbose-option.txt b/Documentation/daxctl/verbose-option.txt new file mode 100644 index 0000000..cb62c8e --- /dev/null +++ b/Documentation/daxctl/verbose-option.txt @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +-v:: +--verbose:: + Emit more debug messages diff --git a/util/filter.c b/util/filter.c index 7c8debb..8c78f32 100644 --- a/util/filter.c +++ b/util/filter.c @@ -342,7 +342,7 @@ struct daxctl_region *util_daxctl_region_filter(struct daxctl_region *region, return region; if ((sscanf(ident, "%d", &region_id) == 1 - || sscanf(ident, "region%d", &region_id) == 1) + || sscanf(ident, "region%d", &region_id) == 1) && daxctl_region_get_id(region) == region_id) return region; diff --git a/test/daxctl-create.sh b/test/daxctl-create.sh index 8dbc00f..a4fbe06 100755 --- a/test/daxctl-create.sh +++ b/test/daxctl-create.sh @@ -20,8 +20,8 @@ find_testdev() # The hmem driver is needed to change the device mode, only # kernels >= v5.6 might have it available. Skip if not. - if ! modinfo hmem; then - # check if hmem is builtin + if ! modinfo dax_hmem; then + # check if dax_hmem is builtin if [ ! -d "/sys/module/device_hmem" ]; then printf "Unable to find hmem module\n" exit $rc @@ -66,7 +66,7 @@ reset_dev() test -n "$testdev" "$DAXCTL" disable-device "$testdev" - "$DAXCTL" reconfigure-device -s $available "$testdev" + "$DAXCTL" reconfigure-device -s "$available" "$testdev" "$DAXCTL" enable-device "$testdev" } @@ -76,7 +76,7 @@ reset() "$DAXCTL" disable-device -r 0 all "$DAXCTL" destroy-device -r 0 all - "$DAXCTL" reconfigure-device -s $available "$testdev" + "$DAXCTL" reconfigure-device -s "$available" "$testdev" } clear_dev() @@ -91,8 +91,8 @@ test_pass() # Available size _available_size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') - if [[ ! $_available_size == $available ]]; then - printf "Unexpected available size $_available_size != $available\n" + if [[ ! $_available_size == "$available" ]]; then + echo "Unexpected available size $_available_size != $available" exit "$rc" fi } @@ -103,7 +103,7 @@ fail_if_available() _size=$("$DAXCTL" list -r 0 | jq -er '.[0].available_size | .//""') if [[ $_size ]]; then - printf "Unexpected available size $_size\n" + echo "Unexpected available size $_size" exit "$rc" fi } @@ -124,8 +124,8 @@ daxctl_get_size_by_mapping() local _start=0 local _end=0 - _start=$(cat $1/start) - _end=$(cat $1/end) + _start=$(cat "$1"/start) + _end=$(cat "$1"/end) ((size=size + _end - _start + 1)) echo $size } @@ -138,10 +138,10 @@ daxctl_get_nr_mappings() local path="" path=$(readlink -f /sys/bus/dax/devices/"$1"/) - until ! [ -d $path/mapping$i ] + until ! [ -d "$path/mapping$i" ] do _size=$(daxctl_get_size_by_mapping "$path/mapping$i") - if [[ $msize == 0 ]]; then + if [[ $_size == 0 ]]; then i=0 break fi @@ -151,8 +151,8 @@ daxctl_get_nr_mappings() # Return number of mappings if the sizes between size field # and the one computed by mappingNNN are the same - _size=$("$DAXCTL" list -d $1 | jq -er '.[0].size | .//""') - if [[ ! $_size == $devsize ]]; then + _size=$("$DAXCTL" list -d "$1" | jq -er '.[0].size | .//""') + if [[ ! $_size == "$devsize" ]]; then echo 0 else echo $i @@ -163,7 +163,7 @@ daxctl_test_multi() { local daxdev - size=$(expr $available / 4) + size=$((available / 4)) if [[ $2 ]]; then "$DAXCTL" disable-device "$testdev" @@ -171,24 +171,24 @@ daxctl_test_multi() fi daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_1 + test -n "$daxdev_1" daxdev_2=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_2 + test -n "$daxdev_2" if [[ ! $2 ]]; then daxdev_3=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test -n $daxdev_3 + test -n "$daxdev_3" fi # Hole - "$DAXCTL" disable-device $1 && "$DAXCTL" destroy-device $1 + "$DAXCTL" disable-device "$1" && "$DAXCTL" destroy-device "$1" # Pick space in the created hole and at the end - new_size=$(expr $size \* 2) - daxdev_4=$("$DAXCTL" create-device -r 0 -s $new_size | jq -er '.[].chardev') - test -n $daxdev_4 - test $(daxctl_get_nr_mappings $daxdev_4) -eq 2 + new_size=$((size * 2)) + daxdev_4=$("$DAXCTL" create-device -r 0 -s "$new_size" | jq -er '.[].chardev') + test -n "$daxdev_4" + test "$(daxctl_get_nr_mappings "$daxdev_4")" -eq 2 fail_if_available @@ -201,7 +201,7 @@ daxctl_test_multi_reconfig() local ncfgs=$1 local daxdev - size=$(expr $available / $ncfgs) + size=$((available / ncfgs)) test -n "$testdev" @@ -212,19 +212,19 @@ daxctl_test_multi_reconfig() daxdev_1=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') "$DAXCTL" disable-device "$daxdev_1" - start=$(expr $size + $size) - max=$(expr $ncfgs / 2 \* $size) + start=$((size + size)) + max=$((size * ncfgs / 2)) for i in $(seq $start $size $max) do "$DAXCTL" disable-device "$testdev" - "$DAXCTL" reconfigure-device -s $i "$testdev" + "$DAXCTL" reconfigure-device -s "$i" "$testdev" "$DAXCTL" disable-device "$daxdev_1" - "$DAXCTL" reconfigure-device -s $i "$daxdev_1" + "$DAXCTL" reconfigure-device -s "$i" "$daxdev_1" done - test $(daxctl_get_nr_mappings $testdev) -eq $((ncfgs / 2)) - test $(daxctl_get_nr_mappings $daxdev_1) -eq $((ncfgs / 2)) + test "$(daxctl_get_nr_mappings "$testdev")" -eq $((ncfgs / 2)) + test "$(daxctl_get_nr_mappings "$daxdev_1")" -eq $((ncfgs / 2)) fail_if_available @@ -237,15 +237,15 @@ daxctl_test_adjust() local ncfgs=4 local daxdev - size=$(expr $available / $ncfgs) + size=$((available / ncfgs)) test -n "$testdev" - start=$(expr $size + $size) + start=$((size + size)) for i in $(seq 1 1 $ncfgs) do - daxdev=$("$DAXCTL" create-device -r 0 -s $size | jq -er '.[].chardev') - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + daxdev=$("$DAXCTL" create-device -r 0 -s "$size" | jq -er '.[].chardev') + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 done daxdev=$(daxctl_get_dev "dax0.1") @@ -255,18 +255,18 @@ daxctl_test_adjust() daxdev=$(daxctl_get_dev "dax0.2") "$DAXCTL" disable-device "$daxdev" - "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + "$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev" # Allocates space at the beginning: expect two mappings as # as don't adjust the mappingX region. This is because we # preserve the relative page_offset of existing allocations - test $(daxctl_get_nr_mappings $daxdev) -eq 2 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 2 daxdev=$(daxctl_get_dev "dax0.3") "$DAXCTL" disable-device "$daxdev" - "$DAXCTL" reconfigure-device -s $(expr $size \* 2) "$daxdev" + "$DAXCTL" reconfigure-device -s $((size * 2)) "$daxdev" # Adjusts space at the end, expect one mapping because we are # able to extend existing region range. - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 fail_if_available @@ -293,7 +293,7 @@ daxctl_test1() daxdev=$("$DAXCTL" create-device -r 0 | jq -er '.[].chardev') test -n "$daxdev" - test $(daxctl_get_nr_mappings $daxdev) -eq 1 + test "$(daxctl_get_nr_mappings "$daxdev")" -eq 1 fail_if_available "$DAXCTL" disable-device "$daxdev" && "$DAXCTL" destroy-device "$daxdev"