Re: [PATCH v6 01/12] mm/sparsemem: Introduce struct mem_section_usage

[Dan Williams]

On Wed, May 1, 2019 at 11:07 PM Dan Williams  wrote:
>
> On Wed, May 1, 2019 at 4:25 PM Pavel Tatashin  
> wrote:
> >
> > On 19-04-17 11:39:00, Dan Williams wrote:
> > > Towards enabling memory hotplug to track partial population of a
> > > section, introduce 'struct mem_section_usage'.
> > >
> > > A pointer to a 'struct mem_section_usage' instance replaces the existing
> > > pointer to a 'pageblock_flags' bitmap. Effectively it adds one more
> > > 'unsigned long' beyond the 'pageblock_flags' (usemap) allocation to
> > > house a new 'map_active' bitmap.  The new bitmap enables the memory
> > > hot{plug,remove} implementation to act on incremental sub-divisions of a
> > > section.
> > >
> > > The primary motivation for this functionality is to support platforms
> > > that mix "System RAM" and "Persistent Memory" within a single section,
> > > or multiple PMEM ranges with different mapping lifetimes within a single
> > > section. The section restriction for hotplug has caused an ongoing saga
> > > of hacks and bugs for devm_memremap_pages() users.
> > >
> > > Beyond the fixups to teach existing paths how to retrieve the 'usemap'
> > > from a section, and updates to usemap allocation path, there are no
> > > expected behavior changes.
> > >
> > > Cc: Michal Hocko 
> > > Cc: Vlastimil Babka 
> > > Cc: Logan Gunthorpe 
> > > Signed-off-by: Dan Williams 
> > > ---
> > >  include/linux/mmzone.h |   23 --
> > >  mm/memory_hotplug.c|   18 ++-
> > >  mm/page_alloc.c|2 +
> > >  mm/sparse.c|   81 
> > > 
> > >  4 files changed, 71 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index 70394cabaf4e..f0bbd85dc19a 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -1160,6 +1160,19 @@ static inline unsigned long 
> > > section_nr_to_pfn(unsigned long sec)
> > >  #define SECTION_ALIGN_UP(pfn)(((pfn) + PAGES_PER_SECTION - 1) & 
> > > PAGE_SECTION_MASK)
> > >  #define SECTION_ALIGN_DOWN(pfn)  ((pfn) & PAGE_SECTION_MASK)
> > >
> > > +#define SECTION_ACTIVE_SIZE ((1UL << SECTION_SIZE_BITS) / BITS_PER_LONG)
> > > +#define SECTION_ACTIVE_MASK (~(SECTION_ACTIVE_SIZE - 1))
> > > +
> > > +struct mem_section_usage {
> > > + /*
> > > +  * SECTION_ACTIVE_SIZE portions of the section that are populated in
> > > +  * the memmap
> > > +  */
> > > + unsigned long map_active;
> >
> > I think this should be proportional to section_size / subsection_size.
> > For example, on intel section size = 128M, and subsection is 2M, so
> > 64bits work nicely. But, on arm64 section size if 1G, so subsection is
> > 16M.
> >
> > On the other hand 16M is already much better than what we have: with 1G
> > section size and 2M pmem alignment we guaranteed to loose 1022M. And
> > with 16M subsection it is only 14M.
>
> I'm ok with it being 16M for now unless it causes a problem in
> practice, i.e. something like the minimum hardware mapping alignment
> for physical memory being less than 16M.

On second thought, arbitrary differences across architectures is a bit
sad. The most common nvdimm namespace alignment granularity is
PMD_SIZE, so perhaps the default sub-section size should try to match
that default.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 16/17] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()

[Brendan Higgins]

> On Thu, May 02, 2019 at 11:45:43AM -0700, Brendan Higgins wrote:
> > On Thu, May 2, 2019 at 11:15 AM  wrote:
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: Greg KH
> > > >
> > > > On Wed, May 01, 2019 at 04:01:25PM -0700, Brendan Higgins wrote:
> > > > > From: Iurii Zaikin 
> > > > >
> > > > > KUnit tests for initialized data behavior of proc_dointvec that is
> > > > > explicitly checked in the code. Includes basic parsing tests including
> > > > > int min/max overflow.
> > > > >
> > > > > Signed-off-by: Iurii Zaikin 
> > > > > Signed-off-by: Brendan Higgins 
> > > > > ---
> > > > >  kernel/Makefile  |   2 +
> > > > >  kernel/sysctl-test.c | 292
> > > > +++
> > > > >  lib/Kconfig.debug|   6 +
> > > > >  3 files changed, 300 insertions(+)
> > > > >  create mode 100644 kernel/sysctl-test.c
> > > > >
> > > > > diff --git a/kernel/Makefile b/kernel/Makefile
> > > > > index 6c57e78817dad..c81a8976b6a4b 100644
> > > > > --- a/kernel/Makefile
> > > > > +++ b/kernel/Makefile
> > > > > @@ -112,6 +112,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o
> > > > >  obj-$(CONFIG_ZONE_DEVICE) += memremap.o
> > > > >  obj-$(CONFIG_RSEQ) += rseq.o
> > > > >
> > > > > +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
> > > >
> > > > You are going to have to have a "standard" naming scheme for test
> > > > modules, are you going to recommend "foo-test" over "test-foo"?  If so,
> > > > that's fine, we should just be consistant and document it somewhere.
> > > >
> > > > Personally, I'd prefer "test-foo", but that's just me, naming is hard...
> > >
> > > My preference would be "test-foo" as well.  Just my 2 cents.
> >
> > I definitely agree we should be consistent. My personal bias
> > (unsurprisingly) is "foo-test," but this is just because that is the
> > convention I am used to in other projects I have worked on.
> >
> > On an unbiased note, we are currently almost evenly split between the
> > two conventions with *slight* preference for "foo-test": I ran the two
> > following grep commands on v5.1-rc7:
> >
> > grep -Hrn --exclude-dir="build" -e "config [a-zA-Z_0-9]\+_TEST$" | wc -l
> > grep -Hrn --exclude-dir="build" -e "config TEST_[a-zA-Z_0-9]\+" | wc -l
> >
> > "foo-test" has 36 occurrences.
> > "test-foo" has 33 occurrences.
> >
> > The things I am more concerned about is how this would affect file
> > naming. If we have a unit test for foo.c, I think foo_test.c is more
> > consistent with our namespacing conventions. The other thing, is if we
> > already have a Kconfig symbol called FOO_TEST (or TEST_FOO) what
> > should we name the KUnit test in this case? FOO_UNIT_TEST?
> > FOO_KUNIT_TEST, like I did above?
>
> Ok, I can live with "foo-test", as you are right, in a directory listing
> and config option, it makes more sense to add it as a suffix.

Cool, so just for future reference, if we already have a Kconfig
symbol called FOO_TEST (or TEST_FOO) what should we name the KUnit
test in this case? FOO_UNIT_TEST? FOO_KUNIT_TEST, like I did above?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Brendan Higgins]

> On 5/2/19 10:36 PM, Brendan Higgins wrote:
> > On Thu, May 2, 2019 at 6:45 PM Frank Rowand  wrote:
> >>
> >> On 5/2/19 4:45 PM, Brendan Higgins wrote:
> >>> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  
> >>> wrote:
> 
>  On 5/2/19 11:07 AM, Brendan Higgins wrote:
> > On Thu, May 2, 2019 at 4:02 AM Greg KH  
> > wrote:
> >>
> >> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
> >>> From: Felix Guo 
> >>>
> >>> The ultimate goal is to create minimal isolated test binaries; in the
> >>> meantime we are using UML to provide the infrastructure to run tests, 
> >>> so
> >>> define an abstract way to configure and run tests that allow us to
> >>> change the context in which tests are built without affecting the 
> >>> user.
> >>> This also makes pretty and dynamic error reporting, and a lot of other
> >>> nice features easier.
> >>>
> >>> kunit_config.py:
> >>>   - parse .config and Kconfig files.
> >>>
> >>> kunit_kernel.py: provides helper functions to:
> >>>   - configure the kernel using kunitconfig.
> >>>   - build the kernel with the appropriate configuration.
> >>>   - provide function to invoke the kernel and stream the output back.
> >>>
> >>> Signed-off-by: Felix Guo 
> >>> Signed-off-by: Brendan Higgins 
> >>
> >> Ah, here's probably my answer to my previous logging format question,
> >> right?  What's the chance that these wrappers output stuff in a 
> >> standard
> >> format that test-framework-tools can already parse?  :)
> >>>
> >>> To be clear, the test-framework-tools format we are talking about is
> >>> TAP13[1], correct?
> >>
> >> I'm not sure what the test community prefers for a format.  I'll let them
> >> jump in and debate that question.
> >>
> >>
> >>>
> >>> My understanding is that is what kselftest is being converted to use.
> >>>
> >
> > It should be pretty easy to do. I had some patches that pack up the
> > results into a serialized format for a presubmit service; it should be
> > pretty straightforward to take the same logic and just change the
> > output format.
> 
>  When examining and trying out the previous versions of the patch I found
>  the wrappers useful to provide information about how to control and use
>  the tests, but I had no interest in using the scripts as they do not
>  fit in with my personal environment and workflow.
> 
>  In the previous versions of the patch, these helper scripts are optional,
>  which is good for my use case.  If the helper scripts are required to
> >>>
> >>> They are still optional.
> >>>
>  get the data into the proper format then the scripts are not quite so
>  optional, they become the expected environment.  I think the proper
>  format should exist without the helper scripts.
> >>>
> >>> That's a good point. A couple things,
> >>>
> >>> First off, supporting TAP13, either in the kernel or the wrapper
> >>> script is not hard, but I don't think that is the real issue that you
> >>> raise.
> >>>
> >>> If your only concern is that you will always be able to have human
> >>> readable KUnit results printed to the kernel log, that is a guarantee
> >>> I feel comfortable making. Beyond that, I think it is going to take a
> >>> long while before I would feel comfortable guaranteeing anything about
> >>> how will KUnit work, what kind of data it will want to expose, and how
> >>> it will be organized. I think the wrapper script provides a nice
> >>> facade that I can maintain, can mediate between the implementation
> >>> details and the user, and can mediate between the implementation
> >>> details and other pieces of software that might want to consume
> >>> results.
> >>>
> >>> [1] https://testanything.org/tap-version-13-specification.html
> >>
> >> My concern is based on a focus on my little part of the world
> >> (which in _previous_ versions of the patch series was the devicetree
> >> unittest.c tests being converted to use the kunit infrastructure).
> >> If I step back and think of the entire kernel globally I may end
> >> up with a different conclusion - but I'm going to remain myopic
> >> for this email.
> >>
> >> I want the test results to be usable by me and my fellow
> >> developers.  I prefer that the test results be easily accessible
> >> (current printk() implementation means that kunit messages are
> >> just as accessible as the current unittest.c printk() output).
> >> If the printk() output needs to be filtered through a script
> >> to generate the actual test results then that is sub-optimal
> >> to me.  It is one more step added to my workflow.  And
> >> potentially with an embedded target a major pain to get a
> >> data file (the kernel log file) transferred from a target
> >> to my development host.
> >
> > That's fair. If that is indeed your only concern, then I don't think
> > the wrapper script will ever be an issue 

[ndctl PATCH 6/8] Documentation/daxctl: add a man page for daxctl-reconfigure-device

[Vishal Verma]

Add a man page describing the new daxctl-reconfigure-device command.

Cc: Pavel Tatashin 
Cc: Dave Hansen 
Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 Documentation/daxctl/Makefile.am  |  3 +-
 .../daxctl/daxctl-reconfigure-device.txt  | 74 +++
 2 files changed, 76 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/daxctl/daxctl-reconfigure-device.txt

diff --git a/Documentation/daxctl/Makefile.am b/Documentation/daxctl/Makefile.am
index 6aba035..715fbad 100644
--- a/Documentation/daxctl/Makefile.am
+++ b/Documentation/daxctl/Makefile.am
@@ -28,7 +28,8 @@ endif
 man1_MANS = \
daxctl.1 \
daxctl-list.1 \
-   daxctl-migrate-device-model.1
+   daxctl-migrate-device-model.1 \
+   daxctl-reconfigure-device.1
 
 CLEANFILES = $(man1_MANS)
 
diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt 
b/Documentation/daxctl/daxctl-reconfigure-device.txt
new file mode 100644
index 000..d79547c
--- /dev/null
+++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+
+daxctl-reconfigure-device(1)
+
+
+NAME
+
+daxctl-reconfigure-device - Reconfigure a dax device into a different mode
+
+SYNOPSIS
+
+[verse]
+'daxctl reconfigure-device'  [...] []
+
+DESCRIPTION
+---
+
+Reconfigure the operational mode of a dax device. This can be used to convert
+a regular 'devdax' mode device to the 'system-ram' mode which allows for the 
dax
+range to be hot-plugged into the system as regular memory.
+
+NOTE: This is a destructive, one-way operation. Any data on the dax device
+*will* be lost, and once reconfigured, there is no equivalent operation to
+go back to the normal 'devdax' mode until a reboot.
+
+OPTIONS
+---
+-r::
+--region=::
+   Restrict the reconfigure 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.
+
+-m::
+--mode=::
+   Specify the mode to which the dax device(s) should be reconfigured.
+   - "system-ram": hotplug the device into system memory. This is a
+ one-way operation. Once a device is configured this way, the
+ setting persists across reboots, until the command is called again
+ to explicitly switch to the 'devdax' mode.
+
+   - "devdax": switch to the normal "device dax" mode. This only takes
+ effect following a system reboot.
+
+-N::
+--no-online::
+   By default, memory sections provided by system-ram devices will be
+   brought online automatically and immediately. Use this option to
+   disable the automatic onlining behavior.
+
+--attempt-offline::
+   When converting from "system-ram" mode to "devdax", it is expected
+   that all the memory sections are first made offline. By default,
+   daxctl won't touch online memory. However with this option, attempt
+   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 for the reconfiguration process
+
+include::../copyright.txt[]
+
+SEE ALSO
+
+linkdaxctl:daxctl-list[1]
-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 5/8] daxctl: add a new reconfigure-device command

[Vishal Verma]

Add a new command 'daxctl-reconfigure-device'. This is used to switch
the mode of a dax device between regular 'device_dax' and
'system-memory'. The command also uses the memory hotplug sysfs
interfaces to online the newly available memory when converting to
'system-ram', and to attempt to offline the memory when converting back
to a DAX device.

Cc: Pavel Tatashin 
Cc: Dave Hansen 
Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 daxctl/Makefile.am |   2 +
 daxctl/builtin.h   |   1 +
 daxctl/daxctl.c|   1 +
 daxctl/device.c| 217 +
 4 files changed, 221 insertions(+)
 create mode 100644 daxctl/device.c

diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index 94f73f9..66dcc7f 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -15,10 +15,12 @@ daxctl_SOURCES =\
daxctl.c \
list.c \
migrate.c \
+   device.c \
../util/json.c
 
 daxctl_LDADD =\
lib/libdaxctl.la \
../libutil.a \
$(UUID_LIBS) \
+   $(KMOD_LIBS) \
$(JSON_LIBS)
diff --git a/daxctl/builtin.h b/daxctl/builtin.h
index 00ef5e9..756ba2a 100644
--- a/daxctl/builtin.h
+++ b/daxctl/builtin.h
@@ -6,4 +6,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);
+int cmd_reconfig_device(int argc, const char **argv, struct daxctl_ctx *ctx);
 #endif /* _DAXCTL_BUILTIN_H_ */
diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index 2e41747..e1ba7b8 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 },
+   { "reconfigure-device", .d_fn = cmd_reconfig_device },
 };
 
 int main(int argc, const char **argv)
diff --git a/daxctl/device.c b/daxctl/device.c
new file mode 100644
index 000..0a6102f
--- /dev/null
+++ b/daxctl/device.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2019 Intel Corporation. All rights reserved. */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static struct {
+   const char *dev;
+   const char *mode;
+   int region_id;
+   bool no_online;
+   bool do_offline;
+   bool human;
+   bool verbose;
+} param = {
+   .region_id = -1,
+};
+
+static int dev_disable(struct daxctl_dev *dev)
+{
+   int rc;
+
+   if (!daxctl_dev_is_enabled(dev))
+   return 0;
+
+   rc = daxctl_dev_disable(dev);
+   if (rc)
+   fprintf(stderr, "%s: disable failed: %s\n",
+   daxctl_dev_get_devname(dev), strerror(-rc));
+
+   return rc;
+}
+
+static int reconfig_mode_ram(struct daxctl_dev *dev)
+{
+   const char *devname = daxctl_dev_get_devname(dev);
+   int rc;
+
+   rc = dev_disable(dev);
+   if (rc)
+   return rc;
+   rc = daxctl_dev_enable_ram(dev);
+   if (rc)
+   return rc;
+
+   if (param.no_online)
+   return 0;
+
+   rc = daxctl_dev_online_node(dev);
+   if (rc < 0) {
+   fprintf(stderr, "%s: unable to online memory: %s\n",
+   devname, strerror(-rc));
+   return rc;
+   }
+   if (param.verbose)
+   fprintf(stderr, "%s: onlined %d memory sections\n",
+   devname, rc);
+
+   return 0;
+}
+
+static int reconfig_mode_devdax(struct daxctl_dev *dev)
+{
+   const char *devname = daxctl_dev_get_devname(dev);
+   int rc;
+
+   if (param.do_offline) {
+   rc = daxctl_dev_offline_node(dev);
+   if (rc < 0) {
+   fprintf(stderr, "%s: unable to offline memory: %s\n",
+   devname, strerror(-rc));
+   return rc;
+   }
+   if (param.verbose)
+   fprintf(stderr, "%s: offlined %d memory sections\n",
+   devname, rc);
+   }
+
+   rc = daxctl_dev_node_is_online(dev);
+   if (rc < 0) {
+   fprintf(stderr, "%s: unable to determine node state: %s\n",
+   devname, strerror(-rc));
+   return rc;
+   }
+   if (rc > 0) {
+   if (param.verbose) {
+   fprintf(stderr, "%s: found %d memory sections online\n",
+   devname, rc);
+   fprintf(stderr, "%s: refusing to change modes\n",
+   devname);
+   }
+   return -EBUSY;
+   }
+
+   rc = dev_disable(dev);
+   if (rc)
+   return rc;
+
+   rc = daxctl_dev_enable_devdax(dev);
+   if (rc)
+   return rc;
+
+   return 0;

[ndctl PATCH 4/8] ndctl: add helpers to get/set the online state for a node

[Vishal Verma]

In preparation for converting DAX devices for use as system-ram via the
kernel's 'kmem' facility, add libndctl helpers to manipulate the sysfs
interfaces to get the target_node of a DAX device, and to online,
offline, and query the state of hotplugged memory sections associated
with a given node.

This adds the following new interfaces:

  daxctl_dev_get_target_node
  daxctl_dev_online_node
  daxctl_dev_offline_node
  daxctl_dev_node_is_online

Cc: Pavel Tatashin 
Cc: Dave Hansen 
Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 daxctl/lib/libdaxctl-private.h |   6 +
 daxctl/lib/libdaxctl.c | 228 +
 daxctl/lib/libdaxctl.sym   |   4 +
 daxctl/libdaxctl.h |   4 +
 4 files changed, 242 insertions(+)

diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl-private.h
index e64d0a7..ef443aa 100644
--- a/daxctl/lib/libdaxctl-private.h
+++ b/daxctl/lib/libdaxctl-private.h
@@ -33,6 +33,11 @@ static const char *dax_modules[] = {
[DAXCTL_DEV_MODE_RAM] = "kmem",
 };
 
+enum node_state {
+   NODE_OFFLINE,
+   NODE_ONLINE,
+};
+
 /**
  * struct daxctl_region - container for dax_devices
  */
@@ -63,6 +68,7 @@ struct daxctl_dev {
struct kmod_module *module;
struct kmod_list *kmod_list;
struct daxctl_region *region;
+   int target_node;
 };
 
 static inline int check_kmod(struct kmod_ctx *kmod_ctx)
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index d50b321..aab2364 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -28,6 +28,7 @@
 #include "libdaxctl-private.h"
 
 static const char *attrs = "dax_region";
+static const char *node_base = "/sys/devices/system/node";
 
 static void free_region(struct daxctl_region *region, struct list_head *head);
 
@@ -397,6 +398,12 @@ static void *add_dax_dev(void *parent, int id, const char 
*daxdev_base)
} else
dbg(ctx, "%s: modalias attribute missing\n", devname);
 
+   sprintf(path, "%s/target_node", daxdev_base);
+   if (sysfs_read_attr(ctx, path, buf) == 0)
+   dev->target_node = strtol(buf, NULL, 0);
+   else
+   dev->target_node = -1;
+
daxctl_dev_foreach(region, dev_dup)
if (dev_dup->id == dev->id) {
free_dev(dev, NULL);
@@ -897,3 +904,224 @@ DAXCTL_EXPORT unsigned long long 
daxctl_dev_get_size(struct daxctl_dev *dev)
 {
return dev->size;
 }
+
+DAXCTL_EXPORT int daxctl_dev_get_target_node(struct daxctl_dev *dev)
+{
+   return dev->target_node;
+}
+
+static int online_one_memblock(struct daxctl_dev *dev, char *path)
+{
+   const char *devname = daxctl_dev_get_devname(dev);
+   struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
+   const char *mode = "online_movable";
+   char buf[SYSFS_ATTR_SIZE];
+   int rc;
+
+   rc = sysfs_read_attr(ctx, path, buf);
+   if (rc) {
+   err(ctx, "%s: Failed to read %s: %s\n",
+   devname, path, strerror(-rc));
+   return rc;
+   }
+
+   /*
+* if already online, possibly due to kernel config or a udev rule,
+* there is nothing to do and we can skip over the memblock
+*/
+   if (strncmp(buf, "online", 6) == 0)
+   return 0;
+
+   rc = sysfs_write_attr_quiet(ctx, path, mode);
+   if (rc)
+   err(ctx, "%s: Failed to online %s: %s\n",
+   devname, path, strerror(-rc));
+   return rc;
+}
+
+static int offline_one_memblock(struct daxctl_dev *dev, char *path)
+{
+   const char *devname = daxctl_dev_get_devname(dev);
+   struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
+   const char *mode = "offline";
+   char buf[SYSFS_ATTR_SIZE];
+   int rc;
+
+   rc = sysfs_read_attr(ctx, path, buf);
+   if (rc) {
+   err(ctx, "%s: Failed to read %s: %s\n",
+   devname, path, strerror(-rc));
+   return rc;
+   }
+
+   /* if already offline, there is nothing to do */
+   if (strncmp(buf, "offline", 6) == 0)
+   return 0;
+
+   rc = sysfs_write_attr_quiet(ctx, path, mode);
+   if (rc)
+   err(ctx, "%s: Failed to offline %s: %s\n",
+   devname, path, strerror(-rc));
+   return rc;
+}
+
+static int daxctl_dev_node_set_state(struct daxctl_dev *dev,
+   enum node_state state)
+{
+   struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
+   int target_node, rc, changed = 0;
+   struct dirent *de;
+   char *node_path;
+   DIR *node_dir;
+
+   target_node = daxctl_dev_get_target_node(dev);
+   if (target_node < 0) {
+   err(ctx, "%s: Unable to get target node\n",
+   daxctl_dev_get_devname(dev));
+   return -ENXIO;
+   }
+
+   rc = asprintf(_path, "%s/node%d", node_base, target_node);
+   if (rc < 0)
+   return 

[ndctl PATCH 8/8] contrib/ndctl: add bash-completion for daxctl-reconfigure-device

[Vishal Verma]

Add bash completion helpers for the new daxctl-reconfigure-device
command.

Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 contrib/ndctl | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/contrib/ndctl b/contrib/ndctl
index d1f8bd6..32c4731 100755
--- a/contrib/ndctl
+++ b/contrib/ndctl
@@ -544,7 +544,7 @@ __daxctlcomp()
 
COMPREPLY=( $( compgen -W "$1" -- "$2" ) )
for cword in "${COMPREPLY[@]}"; do
-   if [[ "$cword" == @(--region|--dev) ]]; then
+   if [[ "$cword" == @(--region|--dev|--mode) ]]; then
COMPREPLY[$i]="${cword}="
else
COMPREPLY[$i]="${cword} "
@@ -569,6 +569,9 @@ __daxctl_comp_options()
--dev)
opts="$(__daxctl_get_devs -i)"
;;
+   --mode)
+   opts="system-ram devdax"
+   ;;
*)
return
;;
@@ -579,8 +582,19 @@ __daxctl_comp_options()
 
 __daxctl_comp_non_option_args()
 {
-   # there aren't any commands that accept non option arguments yet
-   return
+   local subcmd=$1
+   local cur=$2
+   local opts
+
+   case $subcmd in
+   reconfigure-device)
+   opts="$(__daxctl_get_devs -i) all"
+   ;;
+   *)
+   return
+   ;;
+   esac
+   __daxctlcomp "$opts" "$cur"
 }
 
 __daxctl_main()
-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 3/8] daxctl: add libdaxctl interfaces to enable/disable devices

[Vishal Verma]

Add new libdaxctl interfaces to disable a device_dax based devices, and
to enable it into a given mode. The modes available are 'device_dax',
and 'system-ram', where device_dax is the normal device DAX mode used
via a character device, and 'system-ram' uses the kernel's 'kmem'
facility to hotplug the device into the system's memory space, and can
be used as normal system memory.

This adds the following new interfaces:

  daxctl_dev_disable;
  daxctl_dev_enable_devdax;
  daxctl_dev_enable_ram;

Cc: Dave Hansen 
Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 daxctl/lib/Makefile.am |   3 +-
 daxctl/lib/libdaxctl-private.h |  15 ++
 daxctl/lib/libdaxctl.c | 241 +
 daxctl/lib/libdaxctl.sym   |   3 +
 daxctl/libdaxctl.h |   9 ++
 5 files changed, 270 insertions(+), 1 deletion(-)

diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index d3d4852..9f0e444 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -16,7 +16,8 @@ libdaxctl_la_SOURCES =\
libdaxctl.c
 
 libdaxctl_la_LIBADD =\
-   $(UUID_LIBS)
+   $(UUID_LIBS) \
+   $(KMOD_LIBS)
 
 daxctl_modprobe_data_DATA = daxctl.conf
 
diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl-private.h
index 4a462e7..e64d0a7 100644
--- a/daxctl/lib/libdaxctl-private.h
+++ b/daxctl/lib/libdaxctl-private.h
@@ -13,6 +13,8 @@
 #ifndef _LIBDAXCTL_PRIVATE_H_
 #define _LIBDAXCTL_PRIVATE_H_
 
+#include 
+
 #define DAXCTL_EXPORT __attribute__ ((visibility("default")))
 
 enum dax_subsystem {
@@ -26,6 +28,11 @@ static const char *dax_subsystems[] = {
[DAX_BUS] = "/sys/bus/dax/devices",
 };
 
+static const char *dax_modules[] = {
+   [DAXCTL_DEV_MODE_DEVDAX] = "device_dax",
+   [DAXCTL_DEV_MODE_RAM] = "kmem",
+};
+
 /**
  * struct daxctl_region - container for dax_devices
  */
@@ -53,6 +60,14 @@ struct daxctl_dev {
char *dev_path;
struct list_node list;
unsigned long long size;
+   struct kmod_module *module;
+   struct kmod_list *kmod_list;
struct daxctl_region *region;
 };
+
+static inline int check_kmod(struct kmod_ctx *kmod_ctx)
+{
+   return kmod_ctx ? 0 : -ENXIO;
+}
+
 #endif /* _LIBDAXCTL_PRIVATE_H_ */
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index f8f5b8c..d50b321 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -47,6 +47,7 @@ struct daxctl_ctx {
int regions_init;
struct list_head regions;
enum dax_subsystem subsys;
+   struct kmod_ctx *kmod_ctx;
 };
 
 /**
@@ -85,12 +86,20 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct daxctl_ctx 
*ctx, void *userdata)
  */
 DAXCTL_EXPORT int daxctl_new(struct daxctl_ctx **ctx)
 {
+   struct kmod_ctx *kmod_ctx;
struct daxctl_ctx *c;
+   int rc = 0;
 
c = calloc(1, sizeof(struct daxctl_ctx));
if (!c)
return -ENOMEM;
 
+   kmod_ctx = kmod_new(NULL, NULL);
+   if (check_kmod(kmod_ctx) != 0) {
+   rc = -ENXIO;
+   goto out;
+   }
+
c->refcount = 1;
log_init(>ctx, "libdaxctl", "DAXCTL_LOG");
info(c, "ctx %p created\n", c);
@@ -98,8 +107,12 @@ DAXCTL_EXPORT int daxctl_new(struct daxctl_ctx **ctx)
*ctx = c;
list_head_init(>regions);
c->subsys = DAX_UNKNOWN;
+   c->kmod_ctx = kmod_ctx;
 
return 0;
+out:
+   free(c);
+   return rc;
 }
 
 /**
@@ -134,6 +147,7 @@ DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
list_for_each_safe(>regions, region, _r, list)
free_region(region, >regions);
 
+   kmod_unref(ctx->kmod_ctx);
info(ctx, "context %p released\n", ctx);
free(ctx);
 }
@@ -191,6 +205,7 @@ static void free_dev(struct daxctl_dev *dev, struct 
list_head *head)
 {
if (head)
list_del_from(head, >list);
+   kmod_module_unref_list(dev->kmod_list);
free(dev->dev_buf);
free(dev->dev_path);
free(dev);
@@ -308,6 +323,27 @@ DAXCTL_EXPORT struct daxctl_region 
*daxctl_new_region(struct daxctl_ctx *ctx,
return region;
 }
 
+static struct kmod_list *to_module_list(struct daxctl_ctx *ctx,
+   const char *alias)
+{
+   struct kmod_list *list = NULL;
+   int rc;
+
+   if (!ctx->kmod_ctx || !alias)
+   return NULL;
+   if (alias[0] == 0)
+   return NULL;
+
+   rc = kmod_module_new_from_lookup(ctx->kmod_ctx, alias, );
+   if (rc < 0 || !list) {
+   dbg(ctx, "failed to find modules for alias: %s %d list: %s\n",
+   alias, rc, list ? "populated" : "empty");
+   return NULL;
+   }
+
+   return list;
+}
+
 static void *add_dax_dev(void *parent, int id, const char *daxdev_base)
 {
const char *devname = devpath_to_devname(daxdev_base);
@@ -317,6 +353,7 @@ static void *add_dax_dev(void *parent, int id, const char 
*daxdev_base)

[ndctl PATCH 2/8] libdaxctl: cache 'subsystem' in daxctl_ctx

[Vishal Verma]

The 'DAX subsystem' in effect is determined at region or device init
time, and dictates the sysfs base paths for all device/region
operations. In preparation for adding bind/unbind functionality, cache
the subsystem as determined at init time in the library context.

Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 daxctl/lib/libdaxctl.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 70f896b..f8f5b8c 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -46,6 +46,7 @@ struct daxctl_ctx {
void *userdata;
int regions_init;
struct list_head regions;
+   enum dax_subsystem subsys;
 };
 
 /**
@@ -96,6 +97,7 @@ DAXCTL_EXPORT int daxctl_new(struct daxctl_ctx **ctx)
dbg(c, "log_priority=%d\n", c->ctx.log_priority);
*ctx = c;
list_head_init(>regions);
+   c->subsys = DAX_UNKNOWN;
 
return 0;
 }
@@ -454,14 +456,18 @@ static void dax_devices_init(struct daxctl_region *region)
for (i = 0; i < ARRAY_SIZE(dax_subsystems); i++) {
char *region_path;
 
-   if (i == DAX_BUS)
+   if (i == DAX_BUS) {
region_path = region->region_path;
-   else if (i == DAX_CLASS) {
+   if (ctx->subsys == DAX_UNKNOWN)
+   ctx->subsys = DAX_BUS;
+   } else if (i == DAX_CLASS) {
if (asprintf(_path, "%s/dax",
region->region_path) < 0) {
dbg(ctx, "region path alloc fail\n");
continue;
}
+   if (ctx->subsys == DAX_UNKNOWN)
+   ctx->subsys = DAX_CLASS;
} else
continue;
sysfs_device_parse(ctx, region_path, daxdev_fmt, region,
@@ -539,6 +545,8 @@ static void __dax_regions_init(struct daxctl_ctx *ctx, enum 
dax_subsystem subsys
free(dev_path);
if (!region)
err(ctx, "add_dax_region() for %s failed\n", 
de->d_name);
+   if (ctx->subsys == DAX_UNKNOWN)
+   ctx->subsys = subsys;
}
closedir(dir);
 }
-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 7/8] contrib/ndctl: fix region-id completions for daxctl

[Vishal Verma]

The completion helpers for daxctl assumed the region arguments for
specifying daxctl regions were the same as ndctl regions, i.e.
"regionX". This is not true - daxctl region arguments are a simple
numeric 'id'.

Add a new helper __daxctl_get_regions() to complete daxctl region IDs
properly.

While at it, fix a useless use of 'echo' in __daxctl_get_devs() and
quoting in __daxctl_comp_options()

Fixes: d6790a32f32c ("daxctl: Add bash-completion")
Signed-off-by: Vishal Verma 
---
 contrib/ndctl | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/contrib/ndctl b/contrib/ndctl
index e17fb0b..d1f8bd6 100755
--- a/contrib/ndctl
+++ b/contrib/ndctl
@@ -528,8 +528,14 @@ _ndctl()
 
 __daxctl_get_devs()
 {
-   local opts="--devices $*"
-   echo "$(daxctl list $opts | grep -E "^\s*\"chardev\":" | cut -d\" -f4)"
+   local opts=("--devices" "$*")
+   daxctl list "${opts[@]}" | grep -E "^\s*\"chardev\":" | cut -d'"' -f4
+}
+
+__daxctl_get_regions()
+{
+   local opts=("--regions" "$*")
+   daxctl list "${opts[@]}" | grep -E "^\s*\"id\":" | grep -Eo "[0-9]+"
 }
 
 __daxctlcomp()
@@ -558,10 +564,10 @@ __daxctl_comp_options()
local cur_arg=${cur##*=}
case $cur_subopt in
--region)
-   opts=$(__ndctl_get_regions -i)
+   opts="$(__daxctl_get_regions -i)"
;;
--dev)
-   opts=$(__daxctl_get_devs -i)
+   opts="$(__daxctl_get_devs -i)"
;;
*)
return
-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 0/8] daxctl: add a new reconfigure-device command

[Vishal Verma]

Add a new daxctl-reconfigure-device command that lets us reconfigure DAX
devices back and forth between 'system-ram' and 'device-dax' modes. It
also includes facilities to online any newly hot-plugged memory
(default), and attempt to offline memory before converting away from the
system-ram mode (not default, requires a --attempt-offline option).

Currently missing from this series is a way to persistently store which
devices have been 'marked' for use as system-ram. This depends on a
config system overhaul in ndctl, and patches for those will follow
separately and are independent of this work.

Cc: Dan Williams 
Cc: Dave Hansen 
Cc: Pavel Tatashin 


Vishal Verma (8):
  libdaxctl: add interfaces in support of device modes
  libdaxctl: cache 'subsystem' in daxctl_ctx
  daxctl: add libdaxctl interfaces to enable/disable devices
  ndctl: add helpers to get/set the online state for a node
  daxctl: add a new reconfigure-device command
  Documentation/daxctl: add a man page for daxctl-reconfigure-device
  contrib/ndctl: fix region-id completions for daxctl
  contrib/ndctl: add bash-completion for daxctl-reconfigure-device

 Documentation/daxctl/Makefile.am  |   3 +-
 .../daxctl/daxctl-reconfigure-device.txt  |  74 +++
 contrib/ndctl |  34 +-
 daxctl/Makefile.am|   2 +
 daxctl/builtin.h  |   1 +
 daxctl/daxctl.c   |   1 +
 daxctl/device.c   | 217 
 daxctl/lib/Makefile.am|   3 +-
 daxctl/lib/libdaxctl-private.h|  21 +
 daxctl/lib/libdaxctl.c| 511 +-
 daxctl/lib/libdaxctl.sym  |  13 +
 daxctl/libdaxctl.h|  15 +
 12 files changed, 884 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/daxctl/daxctl-reconfigure-device.txt
 create mode 100644 daxctl/device.c

-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH 1/8] libdaxctl: add interfaces in support of device modes

[Vishal Verma]

In preparation for libdaxctl and daxctl to grow operational modes for
DAX devices, add the following supporting APIs:

  daxctl_dev_get_ctx
  daxctl_dev_is_enabled

Cc: Dan Williams 
Signed-off-by: Vishal Verma 
---
 daxctl/lib/libdaxctl.c   | 30 ++
 daxctl/lib/libdaxctl.sym |  6 ++
 daxctl/libdaxctl.h   |  2 ++
 3 files changed, 38 insertions(+)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index c2e3a52..70f896b 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -559,6 +559,36 @@ static void dax_regions_init(struct daxctl_ctx *ctx)
}
 }
 
+static int is_enabled(const char *drvpath)
+{
+   struct stat st;
+
+   if (lstat(drvpath, ) < 0 || !S_ISLNK(st.st_mode))
+   return 0;
+   else
+   return 1;
+}
+
+DAXCTL_EXPORT int daxctl_dev_is_enabled(struct daxctl_dev *dev)
+{
+   struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
+   char *path = dev->dev_buf;
+   int len = dev->buf_len;
+
+   if (snprintf(path, len, "%s/driver", dev->dev_path) >= len) {
+   err(ctx, "%s: buffer too small!\n",
+   daxctl_dev_get_devname(dev));
+   return 0;
+   }
+
+   return is_enabled(path);
+}
+
+DAXCTL_EXPORT struct daxctl_ctx *daxctl_dev_get_ctx(struct daxctl_dev *dev)
+{
+   return dev->region->ctx;
+}
+
 DAXCTL_EXPORT struct daxctl_dev *daxctl_dev_get_first(struct daxctl_region 
*region)
 {
dax_devices_init(region);
diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
index 84d3a69..c4af9a7 100644
--- a/daxctl/lib/libdaxctl.sym
+++ b/daxctl/lib/libdaxctl.sym
@@ -50,3 +50,9 @@ LIBDAXCTL_5 {
 global:
daxctl_region_get_path;
 } LIBDAXCTL_4;
+
+LIBDAXCTL_6 {
+global:
+   daxctl_dev_get_ctx;
+   daxctl_dev_is_enabled;
+} LIBDAXCTL_5;
diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
index 1d13ea2..e20ccb4 100644
--- a/daxctl/libdaxctl.h
+++ b/daxctl/libdaxctl.h
@@ -67,6 +67,8 @@ const char *daxctl_dev_get_devname(struct daxctl_dev *dev);
 int daxctl_dev_get_major(struct daxctl_dev *dev);
 int daxctl_dev_get_minor(struct daxctl_dev *dev);
 unsigned long long daxctl_dev_get_size(struct daxctl_dev *dev);
+struct daxctl_ctx *daxctl_dev_get_ctx(struct daxctl_dev *dev);
+int daxctl_dev_is_enabled(struct daxctl_dev *dev);
 
 #define daxctl_dev_foreach(region, dev) \
 for (dev = daxctl_dev_get_first(region); \
-- 
2.20.1

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v7 03/12] mm/sparsemem: Add helpers track active portions of a section at boot

[Pavel Tatashin]

On 19-05-01 22:55:37, Dan Williams wrote:
> Prepare for hot{plug,remove} of sub-ranges of a section by tracking a
> section active bitmask, each bit representing 2MB (SECTION_SIZE (128M) /
> map_active bitmask length (64)). If it turns out that 2MB is too large
> of an active tracking granularity it is trivial to increase the size of
> the map_active bitmap.
> 
> The implications of a partially populated section is that pfn_valid()
> needs to go beyond a valid_section() check and read the sub-section
> active ranges from the bitmask.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Tested-by: Jane Chu 
> Signed-off-by: Dan Williams 

Hi Dan,

I have sent comments to the previous version of this patch:
https://lore.kernel.org/lkml/ca+ck2bafncvyz956jptnq+aqhjs7uy1zqwfl8fsufwqodkx...@mail.gmail.com/

I think they still apply to this one.

Thank you,
Pasha
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Frank Rowand]

On 5/2/19 10:36 PM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 6:45 PM Frank Rowand  wrote:
>>
>> On 5/2/19 4:45 PM, Brendan Higgins wrote:
>>> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  wrote:

 On 5/2/19 11:07 AM, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 4:02 AM Greg KH  wrote:
>>
>> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
>>> From: Felix Guo 
>>>
>>> The ultimate goal is to create minimal isolated test binaries; in the
>>> meantime we are using UML to provide the infrastructure to run tests, so
>>> define an abstract way to configure and run tests that allow us to
>>> change the context in which tests are built without affecting the user.
>>> This also makes pretty and dynamic error reporting, and a lot of other
>>> nice features easier.
>>>
>>> kunit_config.py:
>>>   - parse .config and Kconfig files.
>>>
>>> kunit_kernel.py: provides helper functions to:
>>>   - configure the kernel using kunitconfig.
>>>   - build the kernel with the appropriate configuration.
>>>   - provide function to invoke the kernel and stream the output back.
>>>
>>> Signed-off-by: Felix Guo 
>>> Signed-off-by: Brendan Higgins 
>>
>> Ah, here's probably my answer to my previous logging format question,
>> right?  What's the chance that these wrappers output stuff in a standard
>> format that test-framework-tools can already parse?  :)
>>>
>>> To be clear, the test-framework-tools format we are talking about is
>>> TAP13[1], correct?
>>
>> I'm not sure what the test community prefers for a format.  I'll let them
>> jump in and debate that question.
>>
>>
>>>
>>> My understanding is that is what kselftest is being converted to use.
>>>
>
> It should be pretty easy to do. I had some patches that pack up the
> results into a serialized format for a presubmit service; it should be
> pretty straightforward to take the same logic and just change the
> output format.

 When examining and trying out the previous versions of the patch I found
 the wrappers useful to provide information about how to control and use
 the tests, but I had no interest in using the scripts as they do not
 fit in with my personal environment and workflow.

 In the previous versions of the patch, these helper scripts are optional,
 which is good for my use case.  If the helper scripts are required to
>>>
>>> They are still optional.
>>>
 get the data into the proper format then the scripts are not quite so
 optional, they become the expected environment.  I think the proper
 format should exist without the helper scripts.
>>>
>>> That's a good point. A couple things,
>>>
>>> First off, supporting TAP13, either in the kernel or the wrapper
>>> script is not hard, but I don't think that is the real issue that you
>>> raise.
>>>
>>> If your only concern is that you will always be able to have human
>>> readable KUnit results printed to the kernel log, that is a guarantee
>>> I feel comfortable making. Beyond that, I think it is going to take a
>>> long while before I would feel comfortable guaranteeing anything about
>>> how will KUnit work, what kind of data it will want to expose, and how
>>> it will be organized. I think the wrapper script provides a nice
>>> facade that I can maintain, can mediate between the implementation
>>> details and the user, and can mediate between the implementation
>>> details and other pieces of software that might want to consume
>>> results.
>>>
>>> [1] https://testanything.org/tap-version-13-specification.html
>>
>> My concern is based on a focus on my little part of the world
>> (which in _previous_ versions of the patch series was the devicetree
>> unittest.c tests being converted to use the kunit infrastructure).
>> If I step back and think of the entire kernel globally I may end
>> up with a different conclusion - but I'm going to remain myopic
>> for this email.
>>
>> I want the test results to be usable by me and my fellow
>> developers.  I prefer that the test results be easily accessible
>> (current printk() implementation means that kunit messages are
>> just as accessible as the current unittest.c printk() output).
>> If the printk() output needs to be filtered through a script
>> to generate the actual test results then that is sub-optimal
>> to me.  It is one more step added to my workflow.  And
>> potentially with an embedded target a major pain to get a
>> data file (the kernel log file) transferred from a target
>> to my development host.
> 
> That's fair. If that is indeed your only concern, then I don't think
> the wrapper script will ever be an issue for you. You will always be
> able to execute a given test the old fashioned/manual way, and the
> wrapper script only summarizes results, it does not change the
> contents.
> 
>>
>> I want a reported test failure to be easy to trace back to the
>> 

Re: [PATCH v2 15/17] MAINTAINERS: add entry for KUnit the unit testing framework

[shuah]

On 5/1/19 5:01 PM, Brendan Higgins wrote:

Add myself as maintainer of KUnit, the Linux kernel's unit testing
framework.

Signed-off-by: Brendan Higgins 
---
  MAINTAINERS | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5c38f21aee787..c78ae95c56b80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8448,6 +8448,16 @@ S:   Maintained
  F:tools/testing/selftests/
  F:Documentation/dev-tools/kselftest*
  
+KERNEL UNIT TESTING FRAMEWORK (KUnit)

+M: Brendan Higgins 
+L: kunit-...@googlegroups.com
+W: https://google.github.io/kunit-docs/third_party/kernel/docs/
+S: Maintained
+F: Documentation/kunit/
+F: include/kunit/
+F: kunit/
+F: tools/testing/kunit/
+


Please add kselftest mailing list to this entry, based on our
conversation on taking these patches through kselftest tree.

thanks,
-- Shuah
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 11/17] kunit: test: add test managed resource tests

[shuah]

On 5/1/19 5:01 PM, Brendan Higgins wrote:

From: Avinash Kondareddy 

Tests how tests interact with test managed resources in their lifetime.

Signed-off-by: Avinash Kondareddy 
Signed-off-by: Brendan Higgins 
---


I think this change log could use more details. It is vague on what it
does.

thanks,
-- Shuah

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 02/12] mm/sparsemem: Introduce common definitions for the size and mask of a section

[Oscar Salvador]

On Fri, May 03, 2019 at 08:57:09AM -0400, Pavel Tatashin wrote:
> On Fri, May 3, 2019 at 6:35 AM Robin Murphy  wrote:
> >
> > On 03/05/2019 01:41, Dan Williams wrote:
> > > On Thu, May 2, 2019 at 7:53 AM Pavel Tatashin  
> > > wrote:
> > >>
> > >> On Wed, Apr 17, 2019 at 2:52 PM Dan Williams  
> > >> wrote:
> > >>>
> > >>> Up-level the local section size and mask from kernel/memremap.c to
> > >>> global definitions.  These will be used by the new sub-section hotplug
> > >>> support.
> > >>>
> > >>> Cc: Michal Hocko 
> > >>> Cc: Vlastimil Babka 
> > >>> Cc: Jérôme Glisse 
> > >>> Cc: Logan Gunthorpe 
> > >>> Signed-off-by: Dan Williams 
> > >>
> > >> Should be dropped from this series as it has been replaced by a very
> > >> similar patch in the mainline:
> > >>
> > >> 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
> > >>   mm/memremap: Rename and consolidate SECTION_SIZE
> > >
> > > I saw that patch fly by and acked it, but I have not seen it picked up
> > > anywhere. I grabbed latest -linus and -next, but don't see that
> > > commit.
> > >
> > > $ git show 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
> > > fatal: bad object 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
> >
> > Yeah, I don't recognise that ID either, nor have I had any notifications
> > that Andrew's picked up anything of mine yet :/
> 
> Sorry for the confusion. I thought I checked in a master branch, but
> turns out I checked in a branch where I applied arm hotremove patches
> and Robin's patch as well. These two patches are essentially the same,
> so which one goes first the other should be dropped.
> 
> Reviewed-by: Pavel Tatashin 

Hey Pavel,

just a friendly note :-) :

you are reviewing v6, I think you might want to review v7 [1] instead ;-)?

[1] https://patchwork.kernel.org/cover/10926035/
 

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 02/12] mm/sparsemem: Introduce common definitions for the size and mask of a section

[Pavel Tatashin]

On Fri, May 3, 2019 at 6:35 AM Robin Murphy  wrote:
>
> On 03/05/2019 01:41, Dan Williams wrote:
> > On Thu, May 2, 2019 at 7:53 AM Pavel Tatashin  
> > wrote:
> >>
> >> On Wed, Apr 17, 2019 at 2:52 PM Dan Williams  
> >> wrote:
> >>>
> >>> Up-level the local section size and mask from kernel/memremap.c to
> >>> global definitions.  These will be used by the new sub-section hotplug
> >>> support.
> >>>
> >>> Cc: Michal Hocko 
> >>> Cc: Vlastimil Babka 
> >>> Cc: Jérôme Glisse 
> >>> Cc: Logan Gunthorpe 
> >>> Signed-off-by: Dan Williams 
> >>
> >> Should be dropped from this series as it has been replaced by a very
> >> similar patch in the mainline:
> >>
> >> 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
> >>   mm/memremap: Rename and consolidate SECTION_SIZE
> >
> > I saw that patch fly by and acked it, but I have not seen it picked up
> > anywhere. I grabbed latest -linus and -next, but don't see that
> > commit.
> >
> > $ git show 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
> > fatal: bad object 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
>
> Yeah, I don't recognise that ID either, nor have I had any notifications
> that Andrew's picked up anything of mine yet :/

Sorry for the confusion. I thought I checked in a master branch, but
turns out I checked in a branch where I applied arm hotremove patches
and Robin's patch as well. These two patches are essentially the same,
so which one goes first the other should be dropped.

Reviewed-by: Pavel Tatashin 

Thank you,
Pasha

>
> Robin.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v7 09/12] mm/sparsemem: Support sub-section hotplug

[Oscar Salvador]

On Wed, May 01, 2019 at 10:56:10PM -0700, Dan Williams wrote:
> The libnvdimm sub-system has suffered a series of hacks and broken
> workarounds for the memory-hotplug implementation's awkward
> section-aligned (128MB) granularity. For example the following backtrace
> is emitted when attempting arch_add_memory() with physical address
> ranges that intersect 'System RAM' (RAM) with 'Persistent Memory' (PMEM)
> within a given section:
> 
>  WARNING: CPU: 0 PID: 558 at kernel/memremap.c:300 
> devm_memremap_pages+0x3b5/0x4c0
>  devm_memremap_pages attempted on mixed region [mem 0x2-0x2fbff 
> flags 0x200]
>  [..]
>  Call Trace:
>dump_stack+0x86/0xc3
>__warn+0xcb/0xf0
>warn_slowpath_fmt+0x5f/0x80
>devm_memremap_pages+0x3b5/0x4c0
>__wrap_devm_memremap_pages+0x58/0x70 [nfit_test_iomap]
>pmem_attach_disk+0x19a/0x440 [nd_pmem]
> 
> Recently it was discovered that the problem goes beyond RAM vs PMEM
> collisions as some platform produce PMEM vs PMEM collisions within a
> given section. The libnvdimm workaround for that case revealed that the
> libnvdimm section-alignment-padding implementation has been broken for a
> long while. A fix for that long-standing breakage introduces as many
> problems as it solves as it would require a backward-incompatible change
> to the namespace metadata interpretation. Instead of that dubious route
> [1], address the root problem in the memory-hotplug implementation.
> 
> [1]: 
> https://lore.kernel.org/r/155000671719.348031.2347363160141119237.st...@dwillia2-desk3.amr.corp.intel.com
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Signed-off-by: Dan Williams 
> ---
>  mm/sparse.c |  223 
> ---
>  1 file changed, 150 insertions(+), 73 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 198371e5fc87..419a3620af6e 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -83,8 +83,15 @@ static int __meminit sparse_index_init(unsigned long 
> section_nr, int nid)
>   unsigned long root = SECTION_NR_TO_ROOT(section_nr);
>   struct mem_section *section;
>  
> + /*
> +  * An existing section is possible in the sub-section hotplug
> +  * case. First hot-add instantiates, follow-on hot-add reuses
> +  * the existing section.
> +  *
> +  * The mem_hotplug_lock resolves the apparent race below.
> +  */
>   if (mem_section[root])
> - return -EEXIST;
> + return 0;

Just a sidenote: we do not bail out on -EEXIST, so it should be fine if we
stick with it.
But if not, I would then clean up sparse_add_section:

--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -901,13 +901,12 @@ int __meminit sparse_add_section(int nid, unsigned long 
start_pfn,
int ret;
 
ret = sparse_index_init(section_nr, nid);
-   if (ret < 0 && ret != -EEXIST)
+   if (ret < 0)
return ret;
 
memmap = section_activate(nid, start_pfn, nr_pages, altmap);
if (IS_ERR(memmap))
return PTR_ERR(memmap);
-   ret = 0;


> +
> + if (!mask)
> + rc = -EINVAL;
> + else if (mask & ms->usage->map_active)

else if (ms->usage->map_active) should be enough?

> + rc = -EEXIST;
> + else
> + ms->usage->map_active |= mask;
> +
> + if (rc) {
> + if (usage)
> + ms->usage = NULL;
> + kfree(usage);
> + return ERR_PTR(rc);
> + }
> +
> + /*
> +  * The early init code does not consider partially populated
> +  * initial sections, it simply assumes that memory will never be
> +  * referenced.  If we hot-add memory into such a section then we
> +  * do not need to populate the memmap and can simply reuse what
> +  * is already there.
> +  */

This puzzles me a bit.
I think we cannot have partially populated early sections, can we?
And how we even come to hot-add memory into those?

Could you please elaborate a bit here?

> + ms = __pfn_to_section(start_pfn);
>   section_mark_present(ms);
> - sparse_init_one_section(ms, section_nr, memmap, usage);
> + sparse_init_one_section(ms, section_nr, memmap, ms->usage);
>  
> -out:
> - if (ret < 0) {
> - kfree(usage);
> - depopulate_section_memmap(start_pfn, PAGES_PER_SECTION, altmap);
> - }
> + if (ret < 0)
> + section_deactivate(start_pfn, nr_pages, nid, altmap);

Uhm, if my eyes do not trick me, ret is only used for the return value from
sparse_index_init(), so this is not needed. Can we get rid of it?

Unfortunately I am running out of time, but I plan to keep reviewing this patch
in the next few days.

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 08/17] kunit: test: add support for test abort

[Logan Gunthorpe]

On 2019-05-03 12:48 a.m., Brendan Higgins wrote:
> On Thu, May 2, 2019 at 8:15 PM Logan Gunthorpe  wrote:
>> On 2019-05-01 5:01 p.m., Brendan Higgins wrote:
>>> +/*
>>> + * struct kunit_try_catch - provides a generic way to run code which might 
>>> fail.
>>> + * @context: used to pass user data to the try and catch functions.
>>> + *
>>> + * kunit_try_catch provides a generic, architecture independent way to 
>>> execute
>>> + * an arbitrary function of type kunit_try_catch_func_t which may bail out 
>>> by
>>> + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, 
>>> @try
>>> + * is stopped at the site of invocation and @catch is catch is called.
>>
>> I found some of the C++ comparisons in this series a bit distasteful but
>> wasn't going to say anything until I saw the try catch But looking
>> into the implementation it's just a thread that can exit early which
>> seems fine to me. Just a poor choice of name I guess...
> 
> Guilty as charged (I have a long history with C++, sorry). Would you
> prefer I changed the name? I just figured that try-catch is a commonly
> understood pattern that describes exactly what I am doing.

It is a commonly understood pattern, but I don't think it's what the
code is doing. Try-catch cleans up an entire stack and allows each level
of the stack to apply local cleanup. This implementation simply exits a
thread and has none of that complexity. To me, it seems like an odd
abstraction here as it's really just a test runner that can exit early
(though I haven't seen the follow-up UML implementation).

I would prefer to see this cleaned up such that the abstraction matches
more what's going on but I don't feel that strongly about it so I'll
leave it up to you to figure out what's best unless other reviewers have
stronger opinions.

>>
>> [snip]
>>
>>> +static void __noreturn kunit_abort(struct kunit *test)
>>> +{
>>> + kunit_set_death_test(test, true);
>>> +
>>> + kunit_try_catch_throw(>try_catch);
>>> +
>>> + /*
>>> +  * Throw could not abort from test.
>>> +  *
>>> +  * XXX: we should never reach this line! As kunit_try_catch_throw is
>>> +  * marked __noreturn.
>>> +  */
>>> + WARN_ONCE(true, "Throw could not abort from test!\n");
>>> +}
>>> +
>>>  int kunit_init_test(struct kunit *test, const char *name)
>>>  {
>>>   spin_lock_init(>lock);
>>> @@ -77,6 +103,7 @@ int kunit_init_test(struct kunit *test, const char *name)
>>>   test->name = name;
>>>   test->vprintk = kunit_vprintk;
>>>   test->fail = kunit_fail;
>>> + test->abort = kunit_abort;
>>
>> There are a number of these function pointers which seem to be pointless
>> to me as you only ever set them to one function. Just call the function
>> directly. As it is, it is an unnecessary indirection for someone reading
>> the code. If and when you have multiple implementations of the function
>> then add the pointer. Don't assume you're going to need it later on and
>> add all this maintenance burden if you never use it..
> 
> Ah, yes, Frank (and probably others) previously asked me to remove
> unnecessary method pointers; I removed all the totally unused ones. As
> for these, I don't use them in this patchset, but I use them in my
> patchsets that will follow up this one. These in particular are
> present so that they can be mocked out for testing.

Adding indirection and function pointers solely for the purpose of
mocking out while testing doesn't sit well with me and I don't think it
should be a pattern that's encouraged. Adding extra complexity like this
to a design to make it unit-testable doesn't seem like something that
makes sense in kernel code. Especially given that indirect calls are
more expensive in the age of spectre.

Also, mocking these particular functions seems like it's an artifact of
how you've designed the try/catch abstraction. If the abstraction was
more around an abort-able test runner then it doesn't make sense to need
to mock out the abort/fail functions as you will be testing overly
generic features of something that don't seem necessary to the
implementation.

>>
>> [snip]
>>
>>> +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch)
>>> +{
>>> + try_catch->run = kunit_generic_run_try_catch;
>>> + try_catch->throw = kunit_generic_throw;
>>> +}
>>
>> Same here. There's only one implementation of try_catch and I can't
>> really see any sensible justification for another implementation. Even
>> if there is, add the indirection when the second implementation is
>> added. This isn't C++ and we don't need to make everything a "method".
> 
> These methods are for a UML specific implementation in a follow up
> patchset, which is needed for some features like crash recovery, death
> tests, and removes dependence on kthreads.
> 
> I know this probably sounds like premature complexity. Arguably it is
> in hindsight, but I wrote those features before I pulled out these
> interfaces (they were actually 

Re: [PATCH v7 08/12] mm/sparsemem: Prepare for sub-section ranges

[Oscar Salvador]

On Wed, May 01, 2019 at 10:56:05PM -0700, Dan Williams wrote:
> Prepare the memory hot-{add,remove} paths for handling sub-section
> ranges by plumbing the starting page frame and number of pages being
> handled through arch_{add,remove}_memory() to
> sparse_{add,remove}_one_section().
> 
> This is simply plumbing, small cleanups, and some identifier renames. No
> intended functional changes.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Signed-off-by: Dan Williams 
> ---
>  include/linux/memory_hotplug.h |7 +-
>  mm/memory_hotplug.c|  118 
> +---
>  mm/sparse.c|7 +-
>  3 files changed, 83 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index ae892eef8b82..835a94650ee3 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -354,9 +354,10 @@ extern int add_memory_resource(int nid, struct resource 
> *resource);
>  extern void move_pfn_range_to_zone(struct zone *zone, unsigned long 
> start_pfn,
>   unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> -   struct vmem_altmap *altmap);
> -extern void sparse_remove_one_section(struct zone *zone, struct mem_section 
> *ms,
> +extern int sparse_add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap);
> +extern void sparse_remove_section(struct zone *zone, struct mem_section *ms,
> + unsigned long pfn, unsigned long nr_pages,
>   unsigned long map_offset, struct vmem_altmap *altmap);
>  extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
> unsigned long pnum);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 108380e20d8f..9f73332af910 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -251,22 +251,44 @@ void __init register_page_bootmem_info_node(struct 
> pglist_data *pgdat)
>  }
>  #endif /* CONFIG_HAVE_BOOTMEM_INFO_NODE */
>  
> -static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
> - struct vmem_altmap *altmap, bool want_memblock)
> +static int __meminit __add_section(int nid, unsigned long pfn,
> + unsigned long nr_pages, struct vmem_altmap *altmap,
> + bool want_memblock)
>  {
>   int ret;
>  
> - if (pfn_valid(phys_start_pfn))
> + if (pfn_valid(pfn))
>   return -EEXIST;
>  
> - ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
> + ret = sparse_add_section(nid, pfn, nr_pages, altmap);
>   if (ret < 0)
>   return ret;
>  
>   if (!want_memblock)
>   return 0;
>  
> - return hotplug_memory_register(nid, __pfn_to_section(phys_start_pfn));
> + return hotplug_memory_register(nid, __pfn_to_section(pfn));
> +}
> +
> +static int subsection_check(unsigned long pfn, unsigned long nr_pages,
> + unsigned long flags, const char *reason)
> +{
> + /*
> +  * Only allow partial section hotplug for !memblock ranges,
> +  * since register_new_memory() requires section alignment, and

What is register_new_memory?

Reviewed-by: Oscar Salvador 

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 00/12] mm: Sub-section memory hotplug support

[Oscar Salvador]

On Thu, May 02, 2019 at 04:20:03PM -0700, Dan Williams wrote:
> On Thu, May 2, 2019 at 3:46 PM Pavel Tatashin  
> wrote:
> >
> > Hi Dan,
> >
> > How do you test these patches? Do you have any instructions?
> 
> Yes, I briefly mentioned this in the cover letter, but here is the
> test I am using:
> 
> >
> > I see for example that check_hotplug_memory_range() still enforces
> > memory_block_size_bytes() alignment.
> >
> > Also, after removing check_hotplug_memory_range(), I tried to online
> > 16M aligned DAX memory, and got the following panic:
> 
> Right, this functionality is currently strictly limited to the
> devm_memremap_pages() case where there are guarantees that the memory
> will never be onlined. This is due to the fact that the section size
> is entangled with the memblock api. That said I would have expected
> you to trigger the warning in subsection_check() before getting this
> far into the hotplug process.
> >
> > # echo online > /sys/devices/system/memory/memory7/state
> > [  202.193132] WARNING: CPU: 2 PID: 351 at drivers/base/memory.c:207
> > memory_block_action+0x110/0x178
> > [  202.193391] Modules linked in:
> > [  202.193698] CPU: 2 PID: 351 Comm: sh Not tainted
> > 5.1.0-rc7_pt_devdax-00038-g865af4385544-dirty #9
> > [  202.193909] Hardware name: linux,dummy-virt (DT)
> > [  202.194122] pstate: 6005 (nZCv daif -PAN -UAO)
> > [  202.194243] pc : memory_block_action+0x110/0x178
> > [  202.194404] lr : memory_block_action+0x90/0x178
> > [  202.194506] sp : 16763ca0
> > [  202.194592] x29: 16763ca0 x28: 80016fd29b80
> > [  202.194724] x27:  x26: 
> > [  202.194838] x25: 15546000 x24: 001c
> > [  202.194949] x23:  x22: 0004
> > [  202.195058] x21: 001c x20: 0008
> > [  202.195168] x19: 0007 x18: 
> > [  202.195281] x17:  x16: 
> > [  202.195393] x15:  x14: 
> > [  202.195505] x13:  x12: 
> > [  202.195614] x11:  x10: 
> > [  202.195744] x9 :  x8 : 00018000
> > [  202.195858] x7 : 0018 x6 : 15541930
> > [  202.195966] x5 : 15541930 x4 : 0001
> > [  202.196074] x3 : 0001 x2 : 
> > [  202.196185] x1 : 0070 x0 : 
> > [  202.196366] Call trace:
> > [  202.196455]  memory_block_action+0x110/0x178
> > [  202.196589]  memory_subsys_online+0x3c/0x80
> > [  202.196681]  device_online+0x6c/0x90
> > [  202.196761]  state_store+0x84/0x100
> > [  202.196841]  dev_attr_store+0x18/0x28
> > [  202.196927]  sysfs_kf_write+0x40/0x58
> > [  202.197010]  kernfs_fop_write+0xcc/0x1d8
> > [  202.197099]  __vfs_write+0x18/0x40
> > [  202.197187]  vfs_write+0xa4/0x1b0
> > [  202.197295]  ksys_write+0x64/0xd8
> > [  202.197430]  __arm64_sys_write+0x18/0x20
> > [  202.197521]  el0_svc_common.constprop.0+0x7c/0xe8
> > [  202.197621]  el0_svc_handler+0x28/0x78
> > [  202.197706]  el0_svc+0x8/0xc
> > [  202.197828] ---[ end trace 57719823dda6d21e ]---

This warning relates to:

for (; section_nr < section_nr_end; section_nr++) {
if (WARN_ON_ONCE(!pfn_valid(pfn)))
return false;

from pages_correctly_probed().
AFAICS, this is orthogonal to subsection_check().


-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v6 02/12] mm/sparsemem: Introduce common definitions for the size and mask of a section

[Robin Murphy]

On 03/05/2019 01:41, Dan Williams wrote:

On Thu, May 2, 2019 at 7:53 AM Pavel Tatashin  wrote:


On Wed, Apr 17, 2019 at 2:52 PM Dan Williams  wrote:


Up-level the local section size and mask from kernel/memremap.c to
global definitions.  These will be used by the new sub-section hotplug
support.

Cc: Michal Hocko 
Cc: Vlastimil Babka 
Cc: Jérôme Glisse 
Cc: Logan Gunthorpe 
Signed-off-by: Dan Williams 


Should be dropped from this series as it has been replaced by a very
similar patch in the mainline:

7c697d7fb5cb14ef60e2b687333ba3efb74f73da
  mm/memremap: Rename and consolidate SECTION_SIZE


I saw that patch fly by and acked it, but I have not seen it picked up
anywhere. I grabbed latest -linus and -next, but don't see that
commit.

$ git show 7c697d7fb5cb14ef60e2b687333ba3efb74f73da
fatal: bad object 7c697d7fb5cb14ef60e2b687333ba3efb74f73da


Yeah, I don't recognise that ID either, nor have I had any notifications 
that Andrew's picked up anything of mine yet :/


Robin.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [v5 2/3] mm/hotplug: make remove_memory() interface useable

[David Hildenbrand]

On 02.05.19 20:43, Pavel Tatashin wrote:
> As of right now remove_memory() interface is inherently broken. It tries
> to remove memory but panics if some memory is not offline. The problem
> is that it is impossible to ensure that all memory blocks are offline as
> this function also takes lock_device_hotplug that is required to
> change memory state via sysfs.
> 

The existing interface can actually work today by registering a hotplug
notifier and rejecting any onlining attempts. But I agree that this way,
the interface becomes more usable.

> So, between calling this function and offlining all memory blocks there
> is always a window when lock_device_hotplug is released, and therefore,
> there is always a chance for a panic during this window.
> 
> Make this interface to return an error if memory removal fails. This way
> it is safe to call this function without panicking machine, and also
> makes it symmetric to add_memory() which already returns an error.
> 
> Signed-off-by: Pavel Tatashin > ---
>  include/linux/memory_hotplug.h |  8 +++--
>  mm/memory_hotplug.c| 61 ++
>  2 files changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 8ade08c50d26..5438a2d92560 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -304,7 +304,7 @@ static inline void pgdat_resize_init(struct pglist_data 
> *pgdat) {}
>  extern bool is_mem_section_removable(unsigned long pfn, unsigned long 
> nr_pages);
>  extern void try_offline_node(int nid);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
> -extern void remove_memory(int nid, u64 start, u64 size);
> +extern int remove_memory(int nid, u64 start, u64 size);
>  extern void __remove_memory(int nid, u64 start, u64 size);
>  
>  #else
> @@ -321,7 +321,11 @@ static inline int offline_pages(unsigned long start_pfn, 
> unsigned long nr_pages)
>   return -EINVAL;
>  }
>  
> -static inline void remove_memory(int nid, u64 start, u64 size) {}
> +static inline bool remove_memory(int nid, u64 start, u64 size)
> +{
> + return -EBUSY;
> +}
> +
>  static inline void __remove_memory(int nid, u64 start, u64 size) {}
>  #endif /* CONFIG_MEMORY_HOTREMOVE */
>  
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 8c454e82d4f6..a826aededa1a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1778,9 +1778,10 @@ static int check_memblock_offlined_cb(struct 
> memory_block *mem, void *arg)
>   endpa = PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1;
>   pr_warn("removing memory fails, because memory [%pa-%pa] is 
> onlined\n",
>   , );
> - }
>  
> - return ret;
> + return -EBUSY;
> + }
> + return 0;
>  }
>  
>  static int check_cpu_on_node(pg_data_t *pgdat)
> @@ -1843,19 +1844,9 @@ void try_offline_node(int nid)
>  }
>  EXPORT_SYMBOL(try_offline_node);
>  
> -/**
> - * remove_memory
> - * @nid: the node ID
> - * @start: physical address of the region to remove
> - * @size: size of the region to remove
> - *
> - * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> - * and online/offline operations before this call, as required by
> - * try_offline_node().
> - */
> -void __ref __remove_memory(int nid, u64 start, u64 size)
> +static int __ref try_remove_memory(int nid, u64 start, u64 size)
>  {
> - int ret;
> + int rc = 0;
>  
>   BUG_ON(check_hotplug_memory_range(start, size));
>  
> @@ -1863,13 +1854,13 @@ void __ref __remove_memory(int nid, u64 start, u64 
> size)
>  
>   /*
>* All memory blocks must be offlined before removing memory.  Check
> -  * whether all memory blocks in question are offline and trigger a BUG()
> +  * whether all memory blocks in question are offline and return error
>* if this is not the case.
>*/
> - ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
> - check_memblock_offlined_cb);
> - if (ret)
> - BUG();
> + rc = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
> +check_memblock_offlined_cb);
> + if (rc)
> + goto done;
>  
>   /* remove memmap entry */
>   firmware_map_remove(start, start + size, "System RAM");
> @@ -1879,14 +1870,42 @@ void __ref __remove_memory(int nid, u64 start, u64 
> size)
>  
>   try_offline_node(nid);
>  
> +done:
>   mem_hotplug_done();
> + return rc;
>  }
>  
> -void remove_memory(int nid, u64 start, u64 size)
> +/**
> + * remove_memory
> + * @nid: the node ID
> + * @start: physical address of the region to remove
> + * @size: size of the region to remove
> + *
> + * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> + * and online/offline operations before this call, as required by
> + * 

Re: [PATCH v7 05/12] mm/sparsemem: Convert kmalloc_section_memmap() to populate_section_memmap()

[Oscar Salvador]

On Wed, May 01, 2019 at 10:55:48PM -0700, Dan Williams wrote:
> Allow sub-section sized ranges to be added to the memmap.
> populate_section_memmap() takes an explict pfn range rather than
> assuming a full section, and those parameters are plumbed all the way
> through to vmmemap_populate(). There should be no sub-section usage in
> current deployments. New warnings are added to clarify which memmap
> allocation paths are sub-section capable.
> 
> Cc: Michal Hocko 
> Cc: David Hildenbrand 
> Cc: Logan Gunthorpe 
> Signed-off-by: Dan Williams 
> ---
>  arch/x86/mm/init_64.c |4 ++-
>  include/linux/mm.h|4 ++-
>  mm/sparse-vmemmap.c   |   21 +++--
>  mm/sparse.c   |   61 
> +++--
>  4 files changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 20d14254b686..bb018d09d2dc 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1457,7 +1457,9 @@ int __meminit vmemmap_populate(unsigned long start, 
> unsigned long end, int node,
>  {
>   int err;
>  
> - if (boot_cpu_has(X86_FEATURE_PSE))
> + if (end - start < PAGES_PER_SECTION * sizeof(struct page))
maybe just:

if (PHYS_PFN(end) - PHYS_PFN(start) < PAGES_PER_SECTION) ?
> + err = vmemmap_populate_basepages(start, end, node);
> + else if (boot_cpu_has(X86_FEATURE_PSE))
>   err = vmemmap_populate_hugepages(start, end, node, altmap);
>   else if (altmap) {
>   pr_err_once("%s: no cpu support for altmap allocations\n",

Although the following looks more clear to me:

int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
struct vmem_altmap *altmap)
{
int err;
bool partial_section = (PHYS_PFN(end) - PFN_PHYS(start)) < 
PAGES_PER_SECTION;

if (partial_section || !boot_cpu_has(X86_FEATURE_PSE))
err = vmemmap_populate_basepages(start, end, node);
else if (boot_cpu_has(X86_FEATURE_PSE))
err = vmemmap_populate_hugepages(start, end, node, altmap);
else if (altmap) {
pr_err_once("%s: no cpu support for altmap allocations\n",
__func__);
err = -ENOMEM;
}

if (!err)
sync_global_pgds(start, end - 1);
return err;
}

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..5360a0e4051d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2748,8 +2748,8 @@ const char * arch_vma_name(struct vm_area_struct *vma);
>  void print_vma_addr(char *prefix, unsigned long rip);
>  
>  void *sparse_buffer_alloc(unsigned long size);
> -struct page *sparse_mem_map_populate(unsigned long pnum, int nid,
> - struct vmem_altmap *altmap);
> +struct page * __populate_section_memmap(unsigned long pfn,
> + unsigned long nr_pages, int nid, struct vmem_altmap *altmap);
>  pgd_t *vmemmap_pgd_populate(unsigned long addr, int node);
>  p4d_t *vmemmap_p4d_populate(pgd_t *pgd, unsigned long addr, int node);
>  pud_t *vmemmap_pud_populate(p4d_t *p4d, unsigned long addr, int node);
> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index 7fec05796796..dcb023aa23d1 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c
> @@ -245,19 +245,26 @@ int __meminit vmemmap_populate_basepages(unsigned long 
> start,
>   return 0;
>  }
>  
> -struct page * __meminit sparse_mem_map_populate(unsigned long pnum, int nid,
> - struct vmem_altmap *altmap)
> +struct page * __meminit __populate_section_memmap(unsigned long pfn,
> + unsigned long nr_pages, int nid, struct vmem_altmap *altmap)
>  {
>   unsigned long start;
>   unsigned long end;
> - struct page *map;
>  
> - map = pfn_to_page(pnum * PAGES_PER_SECTION);
> - start = (unsigned long)map;
> - end = (unsigned long)(map + PAGES_PER_SECTION);
> + /*
> +  * The minimum granularity of memmap extensions is
> +  * SECTION_ACTIVE_SIZE as allocations are tracked in the
> +  * 'map_active' bitmap of the section.
> +  */
> + end = ALIGN(pfn + nr_pages, PHYS_PFN(SECTION_ACTIVE_SIZE));

I would use PAGES_PER_SUB_SECTION directly:

 end = ALIGN(pfn + nr_pages, PAGES_PER_SUB_SECTION);

> + pfn &= PHYS_PFN(SECTION_ACTIVE_MASK);

pfn &= PAGE_SUB_SECTION_MASK ?

[...]
> -static struct page *__kmalloc_section_memmap(void)
> +struct page *populate_section_memmap(unsigned long pfn,
> + unsigned long nr_pages, int nid, struct vmem_altmap *altmap)
>  {
>   struct page *page, *ret;
>   unsigned long memmap_size = sizeof(struct page) * PAGES_PER_SECTION;
>  
> + if ((pfn & ~PAGE_SECTION_MASK) || nr_pages != PAGES_PER_SECTION) {
> + WARN(1, "%s: called with section unaligned parameters\n",
> + __func__);
> + return NULL;
> +  

Re: [PATCH v7 02/12] mm/sparsemem: Introduce common definitions for the size and mask of a section

[Oscar Salvador]

On Wed, May 01, 2019 at 10:55:32PM -0700, Dan Williams wrote:
> Up-level the local section size and mask from kernel/memremap.c to
> global definitions.  These will be used by the new sub-section hotplug
> support.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Jérôme Glisse 
> Cc: Logan Gunthorpe 
> Signed-off-by: Dan Williams 
> ---
>  include/linux/mmzone.h |2 ++
>  kernel/memremap.c  |   10 --
>  mm/hmm.c   |2 --
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index f0bbd85dc19a..6726fc175b51 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1134,6 +1134,8 @@ static inline unsigned long early_pfn_to_nid(unsigned 
> long pfn)
>   * PFN_SECTION_SHIFT pfn to/from section number
>   */
>  #define PA_SECTION_SHIFT (SECTION_SIZE_BITS)
> +#define PA_SECTION_SIZE  (1UL << PA_SECTION_SHIFT)
> +#define PA_SECTION_MASK  (~(PA_SECTION_SIZE-1))

As discussed here [1], we do not need the new PA_SECTION_MASK if we work with
pfns/pages directly, so I'd drop it if you go that way.

Besides that:

Reviewed-by: Oscar Salvador 

[1] https://patchwork.kernel.org/patch/10926047/

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v7 06/12] mm/hotplug: Kill is_dev_zone() usage in __remove_pages()

[Oscar Salvador]

On Wed, May 01, 2019 at 10:55:53PM -0700, Dan Williams wrote:
> The zone type check was a leftover from the cleanup that plumbed altmap
> through the memory hotplug path, i.e. commit da024512a1fa "mm: pass the
> vmem_altmap to arch_remove_memory and __remove_pages".
> 
> Cc: Michal Hocko 
> Cc: Logan Gunthorpe 
> Cc: David Hildenbrand 
> Signed-off-by: Dan Williams 

Reviewed-by: Oscar Salvador 

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v7 01/12] mm/sparsemem: Introduce struct mem_section_usage

[Oscar Salvador]

On Wed, May 01, 2019 at 10:55:27PM -0700, Dan Williams wrote:
> Towards enabling memory hotplug to track partial population of a
> section, introduce 'struct mem_section_usage'.
> 
> A pointer to a 'struct mem_section_usage' instance replaces the existing
> pointer to a 'pageblock_flags' bitmap. Effectively it adds one more
> 'unsigned long' beyond the 'pageblock_flags' (usemap) allocation to
> house a new 'map_active' bitmap.  The new bitmap enables the memory
> hot{plug,remove} implementation to act on incremental sub-divisions of a
> section.
> 
> The primary motivation for this functionality is to support platforms
> that mix "System RAM" and "Persistent Memory" within a single section,
> or multiple PMEM ranges with different mapping lifetimes within a single
> section. The section restriction for hotplug has caused an ongoing saga
> of hacks and bugs for devm_memremap_pages() users.
> 
> Beyond the fixups to teach existing paths how to retrieve the 'usemap'
> from a section, and updates to usemap allocation path, there are no
> expected behavior changes.
> 
> Cc: Michal Hocko 
> Cc: Vlastimil Babka 
> Cc: Logan Gunthorpe 
> Signed-off-by: Dan Williams 

Reviewed-by: Oscar Salvador 

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v7 03/12] mm/sparsemem: Add helpers track active portions of a section at boot

[Oscar Salvador]

On Thu, May 02, 2019 at 07:03:45AM -0700, Dan Williams wrote:
> > section_active_mask() also converts the value to address/size.
> > Why do we need to convert the values and we cannot work with pfn/pages 
> > instead?
> > It should be perfectly possible unless I am missing something.
> >
> > The only thing required would be to export earlier your:
> >
> > +#define PAGES_PER_SUB_SECTION (SECTION_ACTIVE_SIZE / PAGE_SIZE)
> > +#define PAGE_SUB_SECTION_MASK (~(PAGES_PER_SUB_SECTION-1))
> >
> > and change section_active_index to:
> >
> > static inline int section_active_index(unsigned long pfn)
> > {
> > return (pfn & ~(PAGE_SECTION_MASK)) / SUB_SECTION_ACTIVE_PAGES;

Sorry, here I meant:

return (pfn & ~(PAGE_SECTION_MASK)) / PAGES_PER_SUB_SECTION;

But I think you got the idea :-)

-- 
Oscar Salvador
SUSE L3
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 08/17] kunit: test: add support for test abort

[Brendan Higgins]

On Thu, May 2, 2019 at 8:15 PM Logan Gunthorpe  wrote:
>
>
>
> On 2019-05-01 5:01 p.m., Brendan Higgins wrote:
> > +/*
> > + * struct kunit_try_catch - provides a generic way to run code which might 
> > fail.
> > + * @context: used to pass user data to the try and catch functions.
> > + *
> > + * kunit_try_catch provides a generic, architecture independent way to 
> > execute
> > + * an arbitrary function of type kunit_try_catch_func_t which may bail out 
> > by
> > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, 
> > @try
> > + * is stopped at the site of invocation and @catch is catch is called.
>
> I found some of the C++ comparisons in this series a bit distasteful but
> wasn't going to say anything until I saw the try catch But looking
> into the implementation it's just a thread that can exit early which
> seems fine to me. Just a poor choice of name I guess...

Guilty as charged (I have a long history with C++, sorry). Would you
prefer I changed the name? I just figured that try-catch is a commonly
understood pattern that describes exactly what I am doing.

>
> [snip]
>
> > +static void __noreturn kunit_abort(struct kunit *test)
> > +{
> > + kunit_set_death_test(test, true);
> > +
> > + kunit_try_catch_throw(>try_catch);
> > +
> > + /*
> > +  * Throw could not abort from test.
> > +  *
> > +  * XXX: we should never reach this line! As kunit_try_catch_throw is
> > +  * marked __noreturn.
> > +  */
> > + WARN_ONCE(true, "Throw could not abort from test!\n");
> > +}
> > +
> >  int kunit_init_test(struct kunit *test, const char *name)
> >  {
> >   spin_lock_init(>lock);
> > @@ -77,6 +103,7 @@ int kunit_init_test(struct kunit *test, const char *name)
> >   test->name = name;
> >   test->vprintk = kunit_vprintk;
> >   test->fail = kunit_fail;
> > + test->abort = kunit_abort;
>
> There are a number of these function pointers which seem to be pointless
> to me as you only ever set them to one function. Just call the function
> directly. As it is, it is an unnecessary indirection for someone reading
> the code. If and when you have multiple implementations of the function
> then add the pointer. Don't assume you're going to need it later on and
> add all this maintenance burden if you never use it..

Ah, yes, Frank (and probably others) previously asked me to remove
unnecessary method pointers; I removed all the totally unused ones. As
for these, I don't use them in this patchset, but I use them in my
patchsets that will follow up this one. These in particular are
present so that they can be mocked out for testing.

>
> [snip]
>
> > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch)
> > +{
> > + try_catch->run = kunit_generic_run_try_catch;
> > + try_catch->throw = kunit_generic_throw;
> > +}
>
> Same here. There's only one implementation of try_catch and I can't
> really see any sensible justification for another implementation. Even
> if there is, add the indirection when the second implementation is
> added. This isn't C++ and we don't need to make everything a "method".

These methods are for a UML specific implementation in a follow up
patchset, which is needed for some features like crash recovery, death
tests, and removes dependence on kthreads.

I know this probably sounds like premature complexity. Arguably it is
in hindsight, but I wrote those features before I pulled out these
interfaces (they were actually both originally in this patchset, but I
dropped them to make this patchset easier to review). I can remove
these methods and add them back in when I actually use them in the
follow up patchsets if you prefer.

Thanks!
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 16/17] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()

[Greg KH]

On Thu, May 02, 2019 at 11:45:43AM -0700, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 11:15 AM  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Greg KH
> > >
> > > On Wed, May 01, 2019 at 04:01:25PM -0700, Brendan Higgins wrote:
> > > > From: Iurii Zaikin 
> > > >
> > > > KUnit tests for initialized data behavior of proc_dointvec that is
> > > > explicitly checked in the code. Includes basic parsing tests including
> > > > int min/max overflow.
> > > >
> > > > Signed-off-by: Iurii Zaikin 
> > > > Signed-off-by: Brendan Higgins 
> > > > ---
> > > >  kernel/Makefile  |   2 +
> > > >  kernel/sysctl-test.c | 292
> > > +++
> > > >  lib/Kconfig.debug|   6 +
> > > >  3 files changed, 300 insertions(+)
> > > >  create mode 100644 kernel/sysctl-test.c
> > > >
> > > > diff --git a/kernel/Makefile b/kernel/Makefile
> > > > index 6c57e78817dad..c81a8976b6a4b 100644
> > > > --- a/kernel/Makefile
> > > > +++ b/kernel/Makefile
> > > > @@ -112,6 +112,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o
> > > >  obj-$(CONFIG_ZONE_DEVICE) += memremap.o
> > > >  obj-$(CONFIG_RSEQ) += rseq.o
> > > >
> > > > +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
> > >
> > > You are going to have to have a "standard" naming scheme for test
> > > modules, are you going to recommend "foo-test" over "test-foo"?  If so,
> > > that's fine, we should just be consistant and document it somewhere.
> > >
> > > Personally, I'd prefer "test-foo", but that's just me, naming is hard...
> >
> > My preference would be "test-foo" as well.  Just my 2 cents.
> 
> I definitely agree we should be consistent. My personal bias
> (unsurprisingly) is "foo-test," but this is just because that is the
> convention I am used to in other projects I have worked on.
> 
> On an unbiased note, we are currently almost evenly split between the
> two conventions with *slight* preference for "foo-test": I ran the two
> following grep commands on v5.1-rc7:
> 
> grep -Hrn --exclude-dir="build" -e "config [a-zA-Z_0-9]\+_TEST$" | wc -l
> grep -Hrn --exclude-dir="build" -e "config TEST_[a-zA-Z_0-9]\+" | wc -l
> 
> "foo-test" has 36 occurrences.
> "test-foo" has 33 occurrences.
> 
> The things I am more concerned about is how this would affect file
> naming. If we have a unit test for foo.c, I think foo_test.c is more
> consistent with our namespacing conventions. The other thing, is if we
> already have a Kconfig symbol called FOO_TEST (or TEST_FOO) what
> should we name the KUnit test in this case? FOO_UNIT_TEST?
> FOO_KUNIT_TEST, like I did above?

Ok, I can live with "foo-test", as you are right, in a directory listing
and config option, it makes more sense to add it as a suffix.

thanks,

greg k-h
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2 12/17] kunit: tool: add Python wrappers for running KUnit tests

[Greg KH]

On Thu, May 02, 2019 at 04:45:29PM -0700, Brendan Higgins wrote:
> On Thu, May 2, 2019 at 2:16 PM Frank Rowand  wrote:
> >
> > On 5/2/19 11:07 AM, Brendan Higgins wrote:
> > > On Thu, May 2, 2019 at 4:02 AM Greg KH  wrote:
> > >>
> > >> On Wed, May 01, 2019 at 04:01:21PM -0700, Brendan Higgins wrote:
> > >>> From: Felix Guo 
> > >>>
> > >>> The ultimate goal is to create minimal isolated test binaries; in the
> > >>> meantime we are using UML to provide the infrastructure to run tests, so
> > >>> define an abstract way to configure and run tests that allow us to
> > >>> change the context in which tests are built without affecting the user.
> > >>> This also makes pretty and dynamic error reporting, and a lot of other
> > >>> nice features easier.
> > >>>
> > >>> kunit_config.py:
> > >>>   - parse .config and Kconfig files.
> > >>>
> > >>> kunit_kernel.py: provides helper functions to:
> > >>>   - configure the kernel using kunitconfig.
> > >>>   - build the kernel with the appropriate configuration.
> > >>>   - provide function to invoke the kernel and stream the output back.
> > >>>
> > >>> Signed-off-by: Felix Guo 
> > >>> Signed-off-by: Brendan Higgins 
> > >>
> > >> Ah, here's probably my answer to my previous logging format question,
> > >> right?  What's the chance that these wrappers output stuff in a standard
> > >> format that test-framework-tools can already parse?  :)
> 
> To be clear, the test-framework-tools format we are talking about is
> TAP13[1], correct?

Yes.

> My understanding is that is what kselftest is being converted to use.

Yes, and I think it's almost done.  The core of kselftest provides
functions that all tests can use to log messages in the correct format.

The core of kunit should also log the messages in this format as well,
and not rely on the helper scripts as Frank points out, not everyone
will use/want them.  Might as well make it easy for everyone to always
do the right thing and not force it to always be added in later.

thanks,

greg k-h
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm