[daxctl,v2,2/2] daxctl: Opt-in to /sys/bus/dax ABI
diff mbox series

Message ID 154752628167.1710673.10834552344037521034.stgit@dwillia2-desk3.amr.corp.intel.com
State Superseded
Headers show
Series
  • daxctl: Opt-in to /sys/bus/dax ABI
Related show

Commit Message

Dan Williams Jan. 15, 2019, 4:24 a.m. UTC
The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate
device-DAX drivers to be bound to device instances. While the kernel
conversion to '/sys/bus/dax' does not effect the primary ndctl use case
of putting namespaces into 'devdax' mode since that uses libnvdimm
namespace device relative paths, it does break current implementations
of 'ndctl list -X' and 'daxctl list'.  It is also known to break fio and
some pmdk versions that explicitly reference "/sys/class/dax".

In order to avoid userspace regressions the kernel can be configured to
maintain '/sys/class/dax' as the default ABI. However, once all
'/sys/class/dax' users have been converted, or removed from the
installation, an administrator can opt-in to the new '/sys/bus/dax' ABI.
The 'dax migrate-device-model' command installs a modprobe rule to
blacklist the dax_pmem_compat module and arrange for the dax_pmem module
to auto-load in response to the detection of device-DAX instances
emitted from the libnvdimm subsystem.

Reported-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 .gitignore                                         |    1 
 Documentation/daxctl/Makefile.am                   |    3 +
 .../daxctl/daxctl-migrate-device-model.txt         |   47 ++++++++++++++++++++
 configure.ac                                       |    5 ++
 daxctl/Makefile.am                                 |   10 ++++
 daxctl/builtin.h                                   |    1 
 daxctl/daxctl.c                                    |    1 
 daxctl/lib/Makefile.am                             |    2 +
 daxctl/lib/daxctl.conf                             |    2 +
 daxctl/migrate.c                                   |   41 +++++++++++++++++
 ndctl.spec.in                                      |    1 
 11 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/daxctl/daxctl-migrate-device-model.txt
 create mode 100644 daxctl/lib/daxctl.conf
 create mode 100644 daxctl/migrate.c

Comments

Verma, Vishal L Jan. 18, 2019, 1:05 a.m. UTC | #1
Hi Dan,

These mostly look good, just a few minor comments below -

On Mon, 2019-01-14 at 20:24 -0800, Dan Williams wrote:
> The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate
> device-DAX drivers to be bound to device instances. While the kernel
> conversion to '/sys/bus/dax' does not effect the primary ndctl use case

"kernel's conversion" or "kernel converting to"
s/effect/affect/

> of putting namespaces into 'devdax' mode since that uses libnvdimm
> namespace device relative paths, it does break current implementations
> of 'ndctl list -X' and 'daxctl list'.  It is also known to break fio and
> some pmdk versions that explicitly reference "/sys/class/dax".
> 
> In order to avoid userspace regressions the kernel can be configured to
> maintain '/sys/class/dax' as the default ABI. However, once all
> '/sys/class/dax' users have been converted, or removed from the
> installation, an administrator can opt-in to the new '/sys/bus/dax' ABI.
> The 'dax migrate-device-model' command installs a modprobe rule to
> blacklist the dax_pmem_compat module and arrange for the dax_pmem module
> to auto-load in response to the detection of device-DAX instances
> emitted from the libnvdimm subsystem.
> 
> Reported-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  .gitignore                                         |    1 
>  Documentation/daxctl/Makefile.am                   |    3 +
>  .../daxctl/daxctl-migrate-device-model.txt         |   47 ++++++++++++++++++++
>  configure.ac                                       |    5 ++
>  daxctl/Makefile.am                                 |   10 ++++
>  daxctl/builtin.h                                   |    1 
>  daxctl/daxctl.c                                    |    1 
>  daxctl/lib/Makefile.am                             |    2 +
>  daxctl/lib/daxctl.conf                             |    2 +
>  daxctl/migrate.c                                   |   41 +++++++++++++++++
>  ndctl.spec.in                                      |    1 
>  11 files changed, 113 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/daxctl/daxctl-migrate-device-model.txt
>  create mode 100644 daxctl/lib/daxctl.conf
>  create mode 100644 daxctl/migrate.c
> 

[..]

> --- /dev/null
> +++ b/daxctl/migrate.c
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */

Copyright notice can be updated to 2019. This applies the one included
by man pages as well, but that is unrelated and I can take care of that
separately.

> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <daxctl/config.h>
> +#include <daxctl/libdaxctl.h>
> +#include <util/parse-options.h>
> +#include <ccan/array_size/array_size.h>
> +
> +int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx)
> +{
> +	int i;
> +	static const struct option options[] = {
> +		OPT_END(),
> +	};
> +	const char * const u[] = {
> +		"daxctl migrate-device-model",
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, options, u, 0);
> +	for (i = 0; i < argc; i++)
> +		error("unknown parameter \"%s\"\n", argv[i]);
> +
> +	if (argc)
> +		usage_with_options(u, options);
> +
> +	if (symlink(DAXCTL_MODPROBE_DATA, DAXCTL_MODPROBE_INSTALL) == 0) {
> +		fprintf(stderr, " Success: installed %s\n",
> +				DAXCTL_MODPROBE_INSTALL);
> +		return EXIT_SUCCESS;
> +	}
> +
> +	error("failed to install %s: %s\n", DAXCTL_MODPROBE_INSTALL,
> +			strerror(errno));

'Success' is capitalized, but 'failed' is lower case, could make them
consistent. Most other messages seem to be lowercase, including the
'unknown parameter' one above.

> +
> +	return EXIT_FAILURE;
> +}
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index bc4d65c1f988..bc65a471a6d2 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -126,6 +126,7 @@ make check
>  %license util/COPYING licenses/BSD-MIT licenses/CC0
>  %{_bindir}/daxctl
>  %{_mandir}/man1/daxctl*
> +%{_datadir}/daxctl/daxctl.conf
>  
>  %files -n LNAME
>  %defattr(-,root,root)
>
Dan Williams Jan. 18, 2019, 1:32 a.m. UTC | #2
On Thu, Jan 17, 2019 at 5:05 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> Hi Dan,
>
> These mostly look good, just a few minor comments below -
>
> On Mon, 2019-01-14 at 20:24 -0800, Dan Williams wrote:
> > The kernel is implementing a '/sys/bus/dax' ABI to allow for alternate
> > device-DAX drivers to be bound to device instances. While the kernel
> > conversion to '/sys/bus/dax' does not effect the primary ndctl use case
>
> "kernel's conversion" or "kernel converting to"
> s/effect/affect/
>
> > of putting namespaces into 'devdax' mode since that uses libnvdimm
> > namespace device relative paths, it does break current implementations
> > of 'ndctl list -X' and 'daxctl list'.  It is also known to break fio and
> > some pmdk versions that explicitly reference "/sys/class/dax".
> >
> > In order to avoid userspace regressions the kernel can be configured to
> > maintain '/sys/class/dax' as the default ABI. However, once all
> > '/sys/class/dax' users have been converted, or removed from the
> > installation, an administrator can opt-in to the new '/sys/bus/dax' ABI.
> > The 'dax migrate-device-model' command installs a modprobe rule to
> > blacklist the dax_pmem_compat module and arrange for the dax_pmem module
> > to auto-load in response to the detection of device-DAX instances
> > emitted from the libnvdimm subsystem.
> >
> > Reported-by: Jeff Moyer <jmoyer@redhat.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  .gitignore                                         |    1
> >  Documentation/daxctl/Makefile.am                   |    3 +
> >  .../daxctl/daxctl-migrate-device-model.txt         |   47 ++++++++++++++++++++
> >  configure.ac                                       |    5 ++
> >  daxctl/Makefile.am                                 |   10 ++++
> >  daxctl/builtin.h                                   |    1
> >  daxctl/daxctl.c                                    |    1
> >  daxctl/lib/Makefile.am                             |    2 +
> >  daxctl/lib/daxctl.conf                             |    2 +
> >  daxctl/migrate.c                                   |   41 +++++++++++++++++
> >  ndctl.spec.in                                      |    1
> >  11 files changed, 113 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/daxctl/daxctl-migrate-device-model.txt
> >  create mode 100644 daxctl/lib/daxctl.conf
> >  create mode 100644 daxctl/migrate.c
> >
>
> [..]
>
> > --- /dev/null
> > +++ b/daxctl/migrate.c
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
>
> Copyright notice can be updated to 2019. This applies the one included
> by man pages as well, but that is unrelated and I can take care of that
> separately.
>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <daxctl/config.h>
> > +#include <daxctl/libdaxctl.h>
> > +#include <util/parse-options.h>
> > +#include <ccan/array_size/array_size.h>
> > +
> > +int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx)
> > +{
> > +     int i;
> > +     static const struct option options[] = {
> > +             OPT_END(),
> > +     };
> > +     const char * const u[] = {
> > +             "daxctl migrate-device-model",
> > +             NULL
> > +     };
> > +
> > +     argc = parse_options(argc, argv, options, u, 0);
> > +     for (i = 0; i < argc; i++)
> > +             error("unknown parameter \"%s\"\n", argv[i]);
> > +
> > +     if (argc)
> > +             usage_with_options(u, options);
> > +
> > +     if (symlink(DAXCTL_MODPROBE_DATA, DAXCTL_MODPROBE_INSTALL) == 0) {
> > +             fprintf(stderr, " Success: installed %s\n",
> > +                             DAXCTL_MODPROBE_INSTALL);
> > +             return EXIT_SUCCESS;
> > +     }
> > +
> > +     error("failed to install %s: %s\n", DAXCTL_MODPROBE_INSTALL,
> > +                     strerror(errno));
>
> 'Success' is capitalized, but 'failed' is lower case, could make them
> consistent. Most other messages seem to be lowercase, including the
> 'unknown parameter' one above.
>

Good point, I'll respin with the copyright and changelog fixups. Thanks!

Patch
diff mbox series

diff --git a/.gitignore b/.gitignore
index 5a3d8e4507e5..3ef9ff7a9a2f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -18,6 +18,7 @@  Documentation/ndctl/asciidoc.conf
 Documentation/daxctl/asciidoctor-extensions.rb
 Documentation/ndctl/asciidoctor-extensions.rb
 .dirstamp
+daxctl/config.h
 daxctl/daxctl
 daxctl/lib/libdaxctl.la
 daxctl/lib/libdaxctl.lo
diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am
index fc0fbe138d93..6aba035ff543 100644
--- a/Documentation/daxctl/Makefile.am
+++ b/Documentation/daxctl/Makefile.am
@@ -27,7 +27,8 @@  endif
 
 man1_MANS = \
 	daxctl.1 \
-	daxctl-list.1
+	daxctl-list.1 \
+	daxctl-migrate-device-model.1
 
 CLEANFILES = $(man1_MANS)
 
diff --git a/Documentation/daxctl/daxctl-migrate-device-model.txt b/Documentation/daxctl/daxctl-migrate-device-model.txt
new file mode 100644
index 000000000000..23e89f1afafc
--- /dev/null
+++ b/Documentation/daxctl/daxctl-migrate-device-model.txt
@@ -0,0 +1,47 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+daxctl-migrate-device-model(1)
+==============================
+
+NAME
+----
+daxctl-migrate-device-model - Opt-in to the /sys/bus/dax device-model,
+allow for alternative Device-DAX instance drivers.
+
+SYNOPSIS
+--------
+[verse]
+'daxctl migrate-device-model'
+
+Arrange for modprobe to disable the dax_pmem_compat, if present, and
+instead deploy the dax_pmem module to convert to the /sys/bus/dax model.
+Kernel versions prior to v5.1 may not support /sys/bus/dax in which case
+the result of this command is a nop until the kernel is updated.  The
+motivation for changing from /sys/class/dax to /sys/bus/dax is to allow
+for alternative drivers for Device-DAX instances, in particular the
+dax_kmem driver.
+
+By default device-dax publishes a /dev/daxX.Y character device for
+userspace to directly map performance differentiated memory. This is
+fine if the memory is to be exclusively consumed / managed by userspace.
+Alternatively an administrator may want the kernel to manage the memory,
+make it available via malloc(), allow for over-provisioning, and / or
+apply kernel-based resource control schemes to the memory. In that case
+the memory fronted by a given Device-DAX instance can be assigned to the
+dax_kmem driver which arranges for the core-kernel memory-management
+sub-system to assume management of the memory range.
+
+This behavior is opt-in for consideration of existing applications /
+scripts that may be hard coded to use /sys/class/dax. Fixes have been
+submitted to applications known to have these direct dependencies
+http://git.kernel.dk/cgit/fio/commit/?id=b08e7d6b18b4[FIO]
+https://github.com/pmem/pmdk/commit/91bc8620884e[PMDK], however, there may
+be others and a system-owner should be aware of the potential for
+regression of Device-DAX consuming scripts, applications, or older
+daxctl binaries.
+
+The modprobe policy established by this utility becomes effective after
+the next reboot, or after all DAX related modules have been removed and
+re-loaded with "udevadm trigger"
+
+include::../copyright.txt[]
diff --git a/configure.ac b/configure.ac
index a02a2d80e1d5..5b4f1fc8d346 100644
--- a/configure.ac
+++ b/configure.ac
@@ -159,6 +159,11 @@  ndctl_monitorconf=monitor.conf
 AC_SUBST([ndctl_monitorconfdir])
 AC_SUBST([ndctl_monitorconf])
 
+daxctl_modprobe_datadir=${datadir}/daxctl
+daxctl_modprobe_data=daxctl.conf
+AC_SUBST([daxctl_modprobe_datadir])
+AC_SUBST([daxctl_modprobe_data])
+
 my_CFLAGS="\
 -Wall \
 -Wchar-subscripts \
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index fe467d030c38..e424d435c22b 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -2,9 +2,19 @@  include $(top_srcdir)/Makefile.am.in
 
 bin_PROGRAMS = daxctl
 
+DISTCLEANFILES = config.h
+BUILT_SOURCES = config.h
+config.h: Makefile
+	$(AM_V_GEN) echo "/* Autogenerated by daxctl/Makefile.am */" >$@
+	$(AM_V_GEN) echo '#define DAXCTL_MODPROBE_DATA \
+		"$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@
+	$(AM_V_GEN) echo '#define DAXCTL_MODPROBE_INSTALL \
+		"$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
+
 daxctl_SOURCES =\
 		daxctl.c \
 		list.c \
+		migrate.c \
 		../util/json.c
 
 daxctl_LDADD =\
diff --git a/daxctl/builtin.h b/daxctl/builtin.h
index dae2615b7ddb..00ef5e930417 100644
--- a/daxctl/builtin.h
+++ b/daxctl/builtin.h
@@ -5,4 +5,5 @@ 
 
 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);
 #endif /* _DAXCTL_BUILTIN_H_ */
diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index cb6f50e39170..2e41747b5ea9 100644
--- a/daxctl/daxctl.c
+++ b/daxctl/daxctl.c
@@ -70,6 +70,7 @@  static struct cmd_struct commands[] = {
 	{ "version", .d_fn = cmd_version },
 	{ "list", .d_fn = cmd_list },
 	{ "help", .d_fn = cmd_help },
+	{ "migrate-device-model", .d_fn = cmd_migrate },
 };
 
 int main(int argc, const char **argv)
diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index 0167e3995b00..d3d4852916ca 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -18,6 +18,8 @@  libdaxctl_la_SOURCES =\
 libdaxctl_la_LIBADD =\
 	$(UUID_LIBS)
 
+daxctl_modprobe_data_DATA = daxctl.conf
+
 EXTRA_DIST += libdaxctl.sym
 
 libdaxctl_la_LDFLAGS = $(AM_LDFLAGS) \
diff --git a/daxctl/lib/daxctl.conf b/daxctl/lib/daxctl.conf
new file mode 100644
index 000000000000..c64a088cbc0b
--- /dev/null
+++ b/daxctl/lib/daxctl.conf
@@ -0,0 +1,2 @@ 
+blacklist dax_pmem_compat
+alias nd:t7* dax_pmem
diff --git a/daxctl/migrate.c b/daxctl/migrate.c
new file mode 100644
index 000000000000..f7bf8347c4ea
--- /dev/null
+++ b/daxctl/migrate.c
@@ -0,0 +1,41 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2015-2018 Intel Corporation. All rights reserved. */
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdio.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <daxctl/config.h>
+#include <daxctl/libdaxctl.h>
+#include <util/parse-options.h>
+#include <ccan/array_size/array_size.h>
+
+int cmd_migrate(int argc, const char **argv, struct daxctl_ctx *ctx)
+{
+	int i;
+	static const struct option options[] = {
+		OPT_END(),
+	};
+	const char * const u[] = {
+		"daxctl migrate-device-model",
+		NULL
+	};
+
+	argc = parse_options(argc, argv, options, u, 0);
+	for (i = 0; i < argc; i++)
+		error("unknown parameter \"%s\"\n", argv[i]);
+
+	if (argc)
+		usage_with_options(u, options);
+
+	if (symlink(DAXCTL_MODPROBE_DATA, DAXCTL_MODPROBE_INSTALL) == 0) {
+		fprintf(stderr, " Success: installed %s\n",
+				DAXCTL_MODPROBE_INSTALL);
+		return EXIT_SUCCESS;
+	}
+
+	error("failed to install %s: %s\n", DAXCTL_MODPROBE_INSTALL,
+			strerror(errno));
+
+	return EXIT_FAILURE;
+}
diff --git a/ndctl.spec.in b/ndctl.spec.in
index bc4d65c1f988..bc65a471a6d2 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -126,6 +126,7 @@  make check
 %license util/COPYING licenses/BSD-MIT licenses/CC0
 %{_bindir}/daxctl
 %{_mandir}/man1/daxctl*
+%{_datadir}/daxctl/daxctl.conf
 
 %files -n LNAME
 %defattr(-,root,root)