| Message ID | 169525064907.3085225.2583864429793298106.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Commit | c2b28b8ae4256f9ed076c65c18c34d05b275c07c |
| Headers | show |
| Series | [NDCTL,v2] cxl/region: Add -f option for disable-region | expand |
Dave， Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev We noticed that only -f is implemented for disable-memdev, and it left a "TODO: actually detect rather than assume active" in cxl/memdev.c. My questions are: 1. Does the *active* here mean the region(the memdev belongs to) is active ? 2. Is the without force method under developing ? My colleagues(in CC's) are investigating how to gracefully disable-memdev Thanks Zhijian On 21/09/2023 06:57, 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> > > --- > v2: > - Update documentation and help output. (Vishal) > --- > Documentation/cxl/cxl-disable-region.txt | 7 ++++ > cxl/region.c | 49 +++++++++++++++++++++++++++++- > 2 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt > index 6a39aee6ea69..9b98d4d8745a 100644 > --- a/Documentation/cxl/cxl-disable-region.txt > +++ b/Documentation/cxl/cxl-disable-region.txt > @@ -25,6 +25,13 @@ OPTIONS > ------- > include::bus-option.txt[] > > +-f:: > +--force:: > + 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. > + > include::decoder-option.txt[] > > include::debug-option.txt[] > diff --git a/cxl/region.c b/cxl/region.c > index bcd703956207..f8303869727a 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", ¶m.force, > + "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: > >
On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote: > Dave， > > Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev > We noticed that only -f is implemented for disable-memdev, and it left a > "TODO: actually detect rather than assume active" in cxl/memdev.c. > > My questions are: > 1. Does the *active* here mean the region(the memdev belongs to) is active ? > 2. Is the without force method under developing ? > > My colleagues(in CC's) are investigating how to gracefully disable-memdev Zhijian, So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks! > > Thanks > Zhijian > > On 21/09/2023 06:57, 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> >> >> --- >> v2: >> - Update documentation and help output. (Vishal) >> --- >> Documentation/cxl/cxl-disable-region.txt | 7 ++++ >> cxl/region.c | 49 +++++++++++++++++++++++++++++- >> 2 files changed, 55 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt >> index 6a39aee6ea69..9b98d4d8745a 100644 >> --- a/Documentation/cxl/cxl-disable-region.txt >> +++ b/Documentation/cxl/cxl-disable-region.txt >> @@ -25,6 +25,13 @@ OPTIONS >> ------- >> include::bus-option.txt[] >> >> +-f:: >> +--force:: >> + 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. >> + >> include::decoder-option.txt[] >> >> include::debug-option.txt[] >> diff --git a/cxl/region.c b/cxl/region.c >> index bcd703956207..f8303869727a 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", ¶m.force, >> + "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: >>
On 22/09/2023 07:19, Dave Jiang wrote: > > > On 9/20/23 19:58, Zhijian Li (Fujitsu) wrote: >> Dave， >> >> Forgive me for not having a new thread, I'd ask a possible relevant questions about disable-memdev >> We noticed that only -f is implemented for disable-memdev, and it left a >> "TODO: actually detect rather than assume active" in cxl/memdev.c. >> >> My questions are: >> 1. Does the *active* here mean the region(the memdev belongs to) is active ? >> 2. Is the without force method under developing ? >> >> My colleagues(in CC's) are investigating how to gracefully disable-memdev > > Zhijian, > So this was there before the region enumeration showed up according to Dan. Now an update to check if the memdev is part of any active region should be added. Either you guys can send a patch or I can go added it. Let me know. Thanks! Understood, thanks for your information, please go ahead :) I believe our guys are willing to test it. Thanks Zhijian > > >> >> Thanks >> Zhijian >> >> On 21/09/2023 06:57, 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> >>> >>> --- >>> v2: >>> - Update documentation and help output. (Vishal) >>> --- >>> Documentation/cxl/cxl-disable-region.txt | 7 ++++ >>> cxl/region.c | 49 +++++++++++++++++++++++++++++- >>> 2 files changed, 55 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt >>> index 6a39aee6ea69..9b98d4d8745a 100644 >>> --- a/Documentation/cxl/cxl-disable-region.txt >>> +++ b/Documentation/cxl/cxl-disable-region.txt >>> @@ -25,6 +25,13 @@ OPTIONS >>> ------- >>> include::bus-option.txt[] >>> >>> +-f:: >>> +--force:: >>> + 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. >>> + >>> include::decoder-option.txt[] >>> >>> include::debug-option.txt[] >>> diff --git a/cxl/region.c b/cxl/region.c >>> index bcd703956207..f8303869727a 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", ¶m.force, >>> + "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: >>>
On 2023/9/21 6:57, Dave Jiang wrote: > + if (daxctl_memory_online_no_movable(mem)) { > + log_err(&rl, "%s: memory unmovable for %s\n", > + devname, > + daxctl_dev_get_devname(dev)); > + return -EPERM; > + } Hi Dave, It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did. Besides, I send a patch to improve the implementation of daxctl_memory_online_with_zone(). https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u Best Regards, Xiao Yang
On 10/9/23 03:52, Xiao Yang wrote: > On 2023/9/21 6:57, Dave Jiang wrote: >> + if (daxctl_memory_online_no_movable(mem)) { >> + log_err(&rl, "%s: memory unmovable for %s\n", >> + devname, >> + daxctl_dev_get_devname(dev)); >> + return -EPERM; >> + } > Hi Dave, > > It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did. Do you mean: rc = daxctl_memory_online_no_movable(mem); if (rc < 0) return rc; if (rc > 0) { log_err(&rl, "%s memory unmovable for %s\n' ...); return -EPERM; } > > Besides, I send a patch to improve the implementation of daxctl_memory_online_with_zone(). > https://lore.kernel.org/nvdimm/20231009103521.1463-1-yangx.jy@fujitsu.com/T/#u Thanks. I added my review tag. > > Best Regards, > Xiao Yang
On 2023/10/14 6:38, Dave Jiang wrote: > > On 10/9/23 03:52, Xiao Yang wrote: >> On 2023/9/21 6:57, Dave Jiang wrote: >>> + if (daxctl_memory_online_no_movable(mem)) { >>> + log_err(&rl, "%s: memory unmovable for %s\n", >>> + devname, >>> + daxctl_dev_get_devname(dev)); >>> + return -EPERM; >>> + } >> Hi Dave, >> >> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did. > Do you mean: > rc = daxctl_memory_online_no_movable(mem); > if (rc < 0) > return rc; > if (rc > 0) { > log_err(&rl, "%s memory unmovable for %s\n' ...); > return -EPERM; > } > Hi Dave, Sorry for the late reply. Is it necessary to try to online the memory region to the MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just want to check if the onlined memory region is in the MEM_ZONE_NORMAL, the following code seems better: mem->zone = 0; rc = daxctl_memory_op(mem, MEM_GET_ZONE); if (rc < 0) return rc; if (mem->zone == MEM_ZONE_NORMAL) { log_err(&rl, "%s memory unmovable for %s\n' ...); return -EPERM; } Best Regards, Xiao Yang
On 10/29/23 21:33, Xiao Yang wrote: > On 2023/10/14 6:38, Dave Jiang wrote: >> >> On 10/9/23 03:52, Xiao Yang wrote: >>> On 2023/9/21 6:57, Dave Jiang wrote: >>>> + if (daxctl_memory_online_no_movable(mem)) { >>>> + log_err(&rl, "%s: memory unmovable for %s\n", >>>> + devname, >>>> + daxctl_dev_get_devname(dev)); >>>> + return -EPERM; >>>> + } >>> Hi Dave, >>> >>> It seems wrong to check if memory is unmovable by the return number of daxctl_memory_online_no_movable(mem) here. IIRC, the return number of daxctl_memory_online_no_movable(mem)/daxctl_memory_op(MEM_GET_ZONE) indicates how many memory blocks have the same memory zone. So I think you should check mem->zone and MEM_ZONE_NORMAL as daxctl_memory_is_movable() did. >> Do you mean: >> rc = daxctl_memory_online_no_movable(mem); >> if (rc < 0) >> return rc; >> if (rc > 0) { >> log_err(&rl, "%s memory unmovable for %s\n' ...); >> return -EPERM; >> } >> > Hi Dave, > > Sorry for the late reply. > > Is it necessary to try to online the memory region to the MEM_ZONE_NORMAL by daxctl_memory_online_no_movable(mem)? If you just want to check if the onlined memory region is in the MEM_ZONE_NORMAL, the following code seems better: > mem->zone = 0; > rc = daxctl_memory_op(mem, MEM_GET_ZONE); > if (rc < 0) > return rc; > if (mem->zone == MEM_ZONE_NORMAL) { > log_err(&rl, "%s memory unmovable for %s\n' ...); > return -EPERM; > } > Ah that was a mistake. I meant to call the query function and not the online op function. Do you have any objections to if (!daxctl_memory_is_movable(mem)) > Best Regards, > Xiao Yang
Dave Jiang wrote: [..] > Ah that was a mistake. I meant to call the query function and not the > online op function. Do you have any objections to > > if (!daxctl_memory_is_movable(mem)) Wait, why check for movable? ZONE_NORMAL can be removed if you are lucky, and ZONE_MOVABLE may still not be able to be removed if you are unlucky. So I would expect that disable-region attempts to offline all-memory blocks, and if that fails then fail the disable-region. That would of course need to come with documentation that disable-region may leave the memory in a partially offline state. Then the force can just rip the device away with the warning message that physical address space has now been permanently leaked and can not be recovered until a reboot.
On 10/30/23 11:31, Dan Williams wrote: > Dave Jiang wrote: > [..] >> Ah that was a mistake. I meant to call the query function and not the >> online op function. Do you have any objections to >> >> if (!daxctl_memory_is_movable(mem)) > > Wait, why check for movable? ZONE_NORMAL can be removed if you are > lucky, and ZONE_MOVABLE may still not be able to be removed if you are > unlucky. So I would expect that disable-region attempts to offline > all-memory blocks, and if that fails then fail the disable-region. That > would of course need to come with documentation that disable-region may > leave the memory in a partially offline state. Then the force can just > rip the device away with the warning message that physical address space > has now been permanently leaked and can not be recovered until a reboot. I'll update as you outlined above.
diff --git a/Documentation/cxl/cxl-disable-region.txt b/Documentation/cxl/cxl-disable-region.txt index 6a39aee6ea69..9b98d4d8745a 100644 --- a/Documentation/cxl/cxl-disable-region.txt +++ b/Documentation/cxl/cxl-disable-region.txt @@ -25,6 +25,13 @@ OPTIONS ------- include::bus-option.txt[] +-f:: +--force:: + 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. + include::decoder-option.txt[] include::debug-option.txt[] diff --git a/cxl/region.c b/cxl/region.c index bcd703956207..f8303869727a 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", ¶m.force, + "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:
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> --- v2: - Update documentation and help output. (Vishal) --- Documentation/cxl/cxl-disable-region.txt | 7 ++++ cxl/region.c | 49 +++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-)