[NDCTL PATCH] cxl/region: Add -f option for disable-region

[NDCTL PATCH] cxl/region: Add -f option for disable-region

[Dave Jiang]

The current operation for disable_region does not check if the memory
covered by a region is online before attempting to disable the cxl region.
Provide a -f option for the region to force offlining of currently online
memory before disabling the region. Also add a check to fail the operation
entirely if the memory is non-movable.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/cxl/cxl-disable-region.txt |    5 +++
 cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
index 6a39aee6ea69..1778ec3a0f3f 100644
--- a/Documentation/cxl/cxl-disable-region.txt
+++ b/Documentation/cxl/cxl-disable-region.txt
@@ -25,6 +25,11 @@ OPTIONS
 -------
 include::bus-option.txt[]
 
+-f::
+--force::
+	Force offline of memory if they are online before disabling the active
+	region. This does not allow offline of unmovable memory.
+
 include::decoder-option.txt[]
 
 include::debug-option.txt[]
diff --git a/cxl/region.c b/cxl/region.c
index bcd703956207..79a5fb81c257 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -14,6 +14,7 @@
 #include <util/parse-options.h>
 #include <ccan/minmax/minmax.h>
 #include <ccan/short_types/short_types.h>
+#include <daxctl/libdaxctl.h>
 
 #include "filter.h"
 #include "json.h"
@@ -95,6 +96,8 @@ static const struct option enable_options[] = {
 
 static const struct option disable_options[] = {
 	BASE_OPTIONS(),
+	OPT_BOOLEAN('f', "force", &param.force,
+		    "disable region even if memory currently online"),
 	OPT_END(),
 };
 
@@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
 	return cxl_region_delete(region);
 }
 
+static int disable_region(struct cxl_region *region)
+{
+	const char *devname = cxl_region_get_devname(region);
+	struct daxctl_region *dax_region;
+	struct daxctl_memory *mem;
+	struct daxctl_dev *dev;
+	int rc;
+
+	dax_region = cxl_region_get_daxctl_region(region);
+	if (!dax_region)
+		goto out;
+
+	daxctl_dev_foreach(dax_region, dev) {
+		mem = daxctl_dev_get_memory(dev);
+		if (!mem)
+			return -ENXIO;
+
+		if (daxctl_memory_online_no_movable(mem)) {
+			log_err(&rl, "%s: memory unmovable for %s\n",
+					devname,
+					daxctl_dev_get_devname(dev));
+			return -EPERM;
+		}
+
+		/*
+		 * If memory is still online and user wants to force it, attempt
+		 * to offline it.
+		 */
+		if (daxctl_memory_is_online(mem) && param.force) {
+			rc = daxctl_memory_offline(mem);
+			if (rc) {
+				log_err(&rl, "%s: unable to offline %s: %s\n",
+					devname,
+					daxctl_dev_get_devname(dev),
+					strerror(abs(rc)));
+				return rc;
+			}
+		}
+	}
+
+out:
+	return cxl_region_disable(region);
+}
+
 static int do_region_xable(struct cxl_region *region, enum region_actions action)
 {
 	switch (action) {
 	case ACTION_ENABLE:
 		return cxl_region_enable(region);
 	case ACTION_DISABLE:
-		return cxl_region_disable(region);
+		return disable_region(region);
 	case ACTION_DESTROY:
 		return destroy_region(region);
 	default:

Re: [NDCTL PATCH] cxl/region: Add -f option for disable-region

[Verma, Vishal L]

Hi Dave,

Looks good apart from a couple minor nits -

On Tue, 2023-09-19 at 16:52 -0700, Dave Jiang wrote:
> The current operation for disable_region does not check if the memory
> covered by a region is online before attempting to disable the cxl region.
> Provide a -f option for the region to force offlining of currently online
> memory before disabling the region. Also add a check to fail the operation
> entirely if the memory is non-movable.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  Documentation/cxl/cxl-disable-region.txt |    5 +++
>  cxl/region.c                             |   49 +++++++++++++++++++++++++++++-
>  2 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt
> index 6a39aee6ea69..1778ec3a0f3f 100644
> --- a/Documentation/cxl/cxl-disable-region.txt
> +++ b/Documentation/cxl/cxl-disable-region.txt
> @@ -25,6 +25,11 @@ OPTIONS
>  -------
>  include::bus-option.txt[]
>  
> +-f::
> +--force::
> +       Force offline of memory if they are online before disabling the active
> +       region. This does not allow offline of unmovable memory.

Instead of "Force offline of.." perhaps something like - 

Attempt to offline any memory that has been hot-added into the system
via the CXL region before disabling the region. This won't be attempted
if the memory was not added as 'movable', and may still fail even if it
was movable.

- because we can't really force the offlining step, that can always be
refused, even if it is movable.

> +
>  include::decoder-option.txt[]
>  
>  include::debug-option.txt[]
> diff --git a/cxl/region.c b/cxl/region.c
> index bcd703956207..79a5fb81c257 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -14,6 +14,7 @@
>  #include <util/parse-options.h>
>  #include <ccan/minmax/minmax.h>
>  #include <ccan/short_types/short_types.h>
> +#include <daxctl/libdaxctl.h>
>  
>  #include "filter.h"
>  #include "json.h"
> @@ -95,6 +96,8 @@ static const struct option enable_options[] = {
>  
>  static const struct option disable_options[] = {
>         BASE_OPTIONS(),
> +       OPT_BOOLEAN('f', "force", &param.force,
> +                   "disable region even if memory currently online"),

similarly,

"Attempt to offline memory before disabling the region"

>         OPT_END(),
>  };
>  
> @@ -789,13 +792,57 @@ static int destroy_region(struct cxl_region *region)
>         return cxl_region_delete(region);
>  }
>  
> +static int disable_region(struct cxl_region *region)
> +{
> +       const char *devname = cxl_region_get_devname(region);
> +       struct daxctl_region *dax_region;
> +       struct daxctl_memory *mem;
> +       struct daxctl_dev *dev;
> +       int rc;
> +
> +       dax_region = cxl_region_get_daxctl_region(region);
> +       if (!dax_region)
> +               goto out;
> +
> +       daxctl_dev_foreach(dax_region, dev) {
> +               mem = daxctl_dev_get_memory(dev);
> +               if (!mem)
> +                       return -ENXIO;
> +
> +               if (daxctl_memory_online_no_movable(mem)) {
> +                       log_err(&rl, "%s: memory unmovable for %s\n",
> +                                       devname,
> +                                       daxctl_dev_get_devname(dev));
> +                       return -EPERM;
> +               }
> +
> +               /*
> +                * If memory is still online and user wants to force it, attempt
> +                * to offline it.
> +                */
> +               if (daxctl_memory_is_online(mem) && param.force) {
> +                       rc = daxctl_memory_offline(mem);
> +                       if (rc) {
> +                               log_err(&rl, "%s: unable to offline %s: %s\n",
> +                                       devname,
> +                                       daxctl_dev_get_devname(dev),
> +                                       strerror(abs(rc)));
> +                               return rc;
> +                       }
> +               }
> +       }
> +
> +out:
> +       return cxl_region_disable(region);
> +}
> +
>  static int do_region_xable(struct cxl_region *region, enum region_actions action)
>  {
>         switch (action) {
>         case ACTION_ENABLE:
>                 return cxl_region_enable(region);
>         case ACTION_DISABLE:
> -               return cxl_region_disable(region);
> +               return disable_region(region);
>         case ACTION_DESTROY:
>                 return destroy_region(region);
>         default:
> 
>