* [PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations @ 2021-01-21 22:33 Paul Gortmaker 2021-01-21 22:33 ` [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() Paul Gortmaker ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Paul Gortmaker @ 2021-01-21 22:33 UTC (permalink / raw) To: linux-kernel Cc: Paul Gortmaker, Yury Norov, Peter Zijlstra, Paul E. McKenney, Frederic Weisbecker, Josh Triplett, Thomas Gleixner, Ingo Molnar, Li Zefan The basic objective here was to add support for "nohz_full=8-N" and/or "rcu_nocbs="4-N" -- essentially introduce "N" as a portable reference to the last core, evaluated at boot for anything using a CPU list. The thinking behind this, is that people carve off a few early CPUs to support housekeeping tasks, and perhaps dedicate one to a busy I/O peripheral, and then the remaining pool of CPUs out to the end are a part of a commonly configured pool used for the real work the user cares about. Extend that logic out to a fleet of machines - some new, and some nearing EOL, and you've probably got a wide range of core counts to contend with - even though the early number of cores dedicated to the system overhead probably doesn't vary. This change would enable sysadmins to have a common bootarg across all such systems, and would also avoid any off-by-one fencepost errors that happen for users who might briefly forget that core counts start at zero. Looking around before starting, I noticed RCU already had a short-form abbreviation "all" -- but if we want to treat CPU lists in a uniform matter, then tokens shouldn't be implemented at a subsystem level and hence be subsystem specific; each with their own variations. So I moved "all" to global use - for boot args, and for cgroups. Then I added the inverse "none" and finally, the one I wanted -- "N". Originally I did this at the CPU subsys level, but Yury suggested it be moved down further to bitmap level itself, and that was good advice. Things got smaller and less complex. The use of "N" isn't a standalone word like "all" or "none". It will be a part of a complete range specification, possibly with CSV separate ranges before and after; like "nohz_full=5,6,8-N" or "nohz_full=2-N:3/4" Also tested the post-boot cgroup use case as per below: root@hackbox:/sys/fs/cgroup/cpuset# mkdir foo root@hackbox:/sys/fs/cgroup/cpuset# cd foo root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 10-N > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 10-15 root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 0-15 root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# This was on a 16 core machine with CONFIG_NR_CPUS=16 in .config file. Note that "N" is a dynamic quantity, and can change scope if the bitmap is changed in size. So at the risk of stating the obvious, don't use it for "burn_eFuse=128-N" or "secure_erase_firmware=32-N" type stuff. Paul. --- [v1: https://lore.kernel.org/lkml/20210106004850.GA11682@paulmck-ThinkPad-P72/ [v2: push code down from cpu subsys to core bitmap code as per Yury's comments. Change "last" to simply be "N" as per PeterZ. Combination of the two got rid of needing strword() and greatly reduced complexity and footprint of the change -- thanks! ] Cc: Yury Norov <yury.norov@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Josh Triplett <josh@joshtriplett.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: Li Zefan <lizefan@huawei.com> Paul Gortmaker (3): lib: add "all" and "none" as valid ranges to bitmap_parselist() rcu: dont special case "all" handling; let bitmask deal with it lib: support N as end of range in bitmap_parselist() .../admin-guide/kernel-parameters.rst | 15 +++++++++++ .../admin-guide/kernel-parameters.txt | 4 +-- kernel/rcu/tree_plugin.h | 13 +++------ lib/bitmap.c | 27 +++++++++++++++---- 4 files changed, 42 insertions(+), 17 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() 2021-01-21 22:33 [PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations Paul Gortmaker @ 2021-01-21 22:33 ` Paul Gortmaker 2021-01-22 0:07 ` Yury Norov 2021-01-21 22:33 ` [PATCH 2/3] rcu: dont special case "all" handling; let bitmask deal with it Paul Gortmaker 2021-01-21 22:33 ` [PATCH 3/3] lib: support N as end of range in bitmap_parselist() Paul Gortmaker 2 siblings, 1 reply; 10+ messages in thread From: Paul Gortmaker @ 2021-01-21 22:33 UTC (permalink / raw) To: linux-kernel; +Cc: Paul Gortmaker, Yury Norov, Peter Zijlstra, Paul E. McKenney The use of "all" was originally RCU specific - I'd pushed it down to being used for any CPU lists -- then Yuri suggested pushing it down further to be used by any bitmap, which is done here. As a trivial one line extension, we also accept the inverse "none" as a valid alias. Cc: Yury Norov <yury.norov@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> --- Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++ lib/bitmap.c | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst index 682ab28b5c94..5e080080b058 100644 --- a/Documentation/admin-guide/kernel-parameters.rst +++ b/Documentation/admin-guide/kernel-parameters.rst @@ -68,7 +68,18 @@ For example one can add to the command line following parameter: where the final item represents CPUs 100,101,125,126,150,151,... +The following convenience aliases are also accepted and used: + foo_cpus=all + +will provide an full/all-set cpu mask for the associated boot argument. + + foo_cpus=none + +will provide an empty/cleared cpu mask for the associated boot argument. + +Note that "all" and "none" are not necessarily valid/sensible input values +for each available boot parameter expecting a CPU list. This document may not be entirely up to date and comprehensive. The command "modinfo -p ${modulename}" shows a current list of all parameters of a loadable diff --git a/lib/bitmap.c b/lib/bitmap.c index 75006c4036e9..a1010646fbe5 100644 --- a/lib/bitmap.c +++ b/lib/bitmap.c @@ -627,6 +627,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r) * From each group will be used only defined amount of bits. * Syntax: range:used_size/group_size * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 + * Optionally the self-descriptive "all" or "none" can be used. * * Returns: 0 on success, -errno on invalid input strings. Error values: * @@ -640,8 +641,16 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) struct region r; long ret; + if (!strcmp(buf, "all")) { + bitmap_fill(maskp, nmaskbits); + return 0; + } + bitmap_zero(maskp, nmaskbits); + if (!strcmp(buf, "none")) + return 0; + while (buf) { buf = bitmap_find_region(buf); if (buf == NULL) -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() 2021-01-21 22:33 ` [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() Paul Gortmaker @ 2021-01-22 0:07 ` Yury Norov 2021-01-22 4:34 ` Paul Gortmaker 0 siblings, 1 reply; 10+ messages in thread From: Yury Norov @ 2021-01-22 0:07 UTC (permalink / raw) To: Paul Gortmaker Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > > The use of "all" was originally RCU specific - I'd pushed it down to > being used for any CPU lists -- then Yuri suggested pushing it down > further to be used by any bitmap, which is done here. > > As a trivial one line extension, we also accept the inverse "none" > as a valid alias. > > Cc: Yury Norov <yury.norov@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> > --- > Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++ > lib/bitmap.c | 9 +++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst > index 682ab28b5c94..5e080080b058 100644 > --- a/Documentation/admin-guide/kernel-parameters.rst > +++ b/Documentation/admin-guide/kernel-parameters.rst > @@ -68,7 +68,18 @@ For example one can add to the command line following parameter: > > where the final item represents CPUs 100,101,125,126,150,151,... > > +The following convenience aliases are also accepted and used: > > + foo_cpus=all > + > +will provide an full/all-set cpu mask for the associated boot argument. > + > + foo_cpus=none > + > +will provide an empty/cleared cpu mask for the associated boot argument. > + > +Note that "all" and "none" are not necessarily valid/sensible input values > +for each available boot parameter expecting a CPU list. My question from v1 is still there: what about the line like "none,all", ok ",all," or similar? If it's not legal, it should be mentioned in the comment, if it is legal, the corresponding code should go to bitmap_parse_region(), just like for "N". My personal preference is the latter option. > This document may not be entirely up to date and comprehensive. The command > "modinfo -p ${modulename}" shows a current list of all parameters of a loadable > diff --git a/lib/bitmap.c b/lib/bitmap.c > index 75006c4036e9..a1010646fbe5 100644 > --- a/lib/bitmap.c > +++ b/lib/bitmap.c > @@ -627,6 +627,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > * From each group will be used only defined amount of bits. > * Syntax: range:used_size/group_size > * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 > + * Optionally the self-descriptive "all" or "none" can be used. > * > * Returns: 0 on success, -errno on invalid input strings. Error values: > * > @@ -640,8 +641,16 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > struct region r; > long ret; > > + if (!strcmp(buf, "all")) { > + bitmap_fill(maskp, nmaskbits); > + return 0; > + } > + > bitmap_zero(maskp, nmaskbits); > > + if (!strcmp(buf, "none")) > + return 0; > + > while (buf) { > buf = bitmap_find_region(buf); > if (buf == NULL) > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() 2021-01-22 0:07 ` Yury Norov @ 2021-01-22 4:34 ` Paul Gortmaker 0 siblings, 0 replies; 10+ messages in thread From: Paul Gortmaker @ 2021-01-22 4:34 UTC (permalink / raw) To: Yury Norov; +Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney [Re: [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist()] On 21/01/2021 (Thu 16:07) Yury Norov wrote: > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker > <paul.gortmaker@windriver.com> wrote: > > > > The use of "all" was originally RCU specific - I'd pushed it down to > > being used for any CPU lists -- then Yuri suggested pushing it down > > further to be used by any bitmap, which is done here. > > > > As a trivial one line extension, we also accept the inverse "none" > > as a valid alias. > > > > Cc: Yury Norov <yury.norov@gmail.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> > > --- > > Documentation/admin-guide/kernel-parameters.rst | 11 +++++++++++ > > lib/bitmap.c | 9 +++++++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst > > index 682ab28b5c94..5e080080b058 100644 > > --- a/Documentation/admin-guide/kernel-parameters.rst > > +++ b/Documentation/admin-guide/kernel-parameters.rst > > @@ -68,7 +68,18 @@ For example one can add to the command line following parameter: > > > > where the final item represents CPUs 100,101,125,126,150,151,... > > > > +The following convenience aliases are also accepted and used: > > > > + foo_cpus=all > > + > > +will provide an full/all-set cpu mask for the associated boot argument. > > + > > + foo_cpus=none > > + > > +will provide an empty/cleared cpu mask for the associated boot argument. > > + > > +Note that "all" and "none" are not necessarily valid/sensible input values > > +for each available boot parameter expecting a CPU list. > > My question from v1 is still there: what about the line like > "none,all", ok ",all," Apologies - I must have overlooked that somehow. Let me address it now. > or similar? If it's not legal, it should be mentioned in the comment, OK, it is not legal. So if desired, I can do this in the code... - * Optionally the self-descriptive "all" or "none" can be used. + * Optionally the self-descriptive stand alone "all" or "none" can be used. ...and a similar "stand alone" addition in kernel-parameters.rst above? > if it is legal, > the corresponding code should go to bitmap_parse_region(), just like for "N". Non-standalone is not legal. The strcmp ensures the "all" or "none" are stand-alone. And as can be seen in the testing below, any attempt to combine them with commas or ranges or repeated instances is -EINVAL. (And I'll look at adding such tests to bitmap_test.c as requested.) > My personal preference is the latter option. I'm a bit confused as to the value in adding code for supporting things like ",all,none,all,,none" and then having to define some policy, like "last processed takes precedence" or similar. A strict stand-alone "all" or "none" and everything else as -EINVAL as per below seems logical. Maybe I'm missing something and you can elaborate? Thanks Paul. -- root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all,none,all > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none,all > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all,all > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all, > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo ,none > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 1,3,5,7,9,11,13,15 > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 1,3,5,7,9,11,13,15 root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 1,3,5,7,9,11,13,all > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none,3,5,7,9,11,13,15 > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] rcu: dont special case "all" handling; let bitmask deal with it 2021-01-21 22:33 [PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations Paul Gortmaker 2021-01-21 22:33 ` [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() Paul Gortmaker @ 2021-01-21 22:33 ` Paul Gortmaker 2021-01-21 22:33 ` [PATCH 3/3] lib: support N as end of range in bitmap_parselist() Paul Gortmaker 2 siblings, 0 replies; 10+ messages in thread From: Paul Gortmaker @ 2021-01-21 22:33 UTC (permalink / raw) To: linux-kernel; +Cc: Paul Gortmaker, Yury Norov, Peter Zijlstra, Paul E. McKenney Now that the core bitmap parse code respects the "all" parameter, there is no need for RCU to have its own special check for it. Cc: Yury Norov <yury.norov@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> --- Documentation/admin-guide/kernel-parameters.txt | 4 +--- kernel/rcu/tree_plugin.h | 13 ++++--------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a10b545c2070..a116c0ff0a91 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -4037,9 +4037,7 @@ see CONFIG_RAS_CEC help text. rcu_nocbs= [KNL] - The argument is a cpu list, as described above, - except that the string "all" can be used to - specify every CPU on the system. + The argument is a cpu list, as described above. In kernels built with CONFIG_RCU_NOCB_CPU=y, set the specified list of CPUs to be no-callback CPUs. diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h index 7e291ce0a1d6..642ebd6569c7 100644 --- a/kernel/rcu/tree_plugin.h +++ b/kernel/rcu/tree_plugin.h @@ -1463,20 +1463,15 @@ static void rcu_cleanup_after_idle(void) /* * Parse the boot-time rcu_nocb_mask CPU list from the kernel parameters. - * The string after the "rcu_nocbs=" is either "all" for all CPUs, or a - * comma-separated list of CPUs and/or CPU ranges. If an invalid list is - * given, a warning is emitted and all CPUs are offloaded. + * If the list is invalid, a warning is emitted and all CPUs are offloaded. */ static int __init rcu_nocb_setup(char *str) { alloc_bootmem_cpumask_var(&rcu_nocb_mask); - if (!strcasecmp(str, "all")) + if (cpulist_parse(str, rcu_nocb_mask)) { + pr_warn("rcu_nocbs= bad CPU range, all CPUs set\n"); cpumask_setall(rcu_nocb_mask); - else - if (cpulist_parse(str, rcu_nocb_mask)) { - pr_warn("rcu_nocbs= bad CPU range, all CPUs set\n"); - cpumask_setall(rcu_nocb_mask); - } + } return 1; } __setup("rcu_nocbs=", rcu_nocb_setup); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] lib: support N as end of range in bitmap_parselist() 2021-01-21 22:33 [PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations Paul Gortmaker 2021-01-21 22:33 ` [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() Paul Gortmaker 2021-01-21 22:33 ` [PATCH 2/3] rcu: dont special case "all" handling; let bitmask deal with it Paul Gortmaker @ 2021-01-21 22:33 ` Paul Gortmaker 2021-01-22 0:29 ` Yury Norov 2 siblings, 1 reply; 10+ messages in thread From: Paul Gortmaker @ 2021-01-21 22:33 UTC (permalink / raw) To: linux-kernel; +Cc: Paul Gortmaker, Yury Norov, Peter Zijlstra, Paul E. McKenney While this is done for all bitmaps, the original use case in mind was for CPU masks and cpulist_parse(). Credit to Yury who suggested to push it down from CPU subsys to bitmap - it simplified things a lot. It seems that a common configuration is to use the 1st couple cores for housekeeping tasks, and or driving a busy peripheral that generates a lot of interrupts, or something similar. This tends to leave the remaining ones to form a pool of similarly configured cores to take on the real workload of interest to the user. So on machine A - with 32 cores, it could be 0-3 for "system" and then 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of setting up the worker pool of CPUs. But then newer machine B is added, and it has 48 cores, and so while the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47. Deployment would be easier if we could just simply replace 31 and 47 with "N" and let the system substitute in the actual number at boot; a number that it knows better than we do. No need to have custom boot args per node, no need to do a trial boot in order to snoop /proc/cpuinfo and/or /sys/devices/system/cpu - no more fencepost errors of using 32 and 48 instead of 31 and 47. Cc: Yury Norov <yury.norov@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> --- .../admin-guide/kernel-parameters.rst | 4 ++++ lib/bitmap.c | 18 +++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst index 5e080080b058..668f0b69fb4f 100644 --- a/Documentation/admin-guide/kernel-parameters.rst +++ b/Documentation/admin-guide/kernel-parameters.rst @@ -68,6 +68,10 @@ For example one can add to the command line following parameter: where the final item represents CPUs 100,101,125,126,150,151,... +The value "N" can be used as the end of a range, to represent the numerically +last CPU on the system, i.e "foo_cpus=16-N" would be equivalent to "16-31" on +a 32 core system. + The following convenience aliases are also accepted and used: foo_cpus=all diff --git a/lib/bitmap.c b/lib/bitmap.c index a1010646fbe5..d498ea9d526b 100644 --- a/lib/bitmap.c +++ b/lib/bitmap.c @@ -571,7 +571,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end return end; } -static const char *bitmap_parse_region(const char *str, struct region *r) +static const char *bitmap_parse_region(const char *str, struct region *r, int nmaskbits) { str = bitmap_getnum(str, &r->start); if (IS_ERR(str)) @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r) if (*str != '-') return ERR_PTR(-EINVAL); - str = bitmap_getnum(str + 1, &r->end); - if (IS_ERR(str)) - return str; + str++; + if (*str == 'N') { + r->end = nmaskbits - 1; + str++; + } else { + str = bitmap_getnum(str, &r->end); + if (IS_ERR(str)) + return str; + } if (end_of_region(*str)) goto no_pattern; @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r) * Syntax: range:used_size/group_size * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 * Optionally the self-descriptive "all" or "none" can be used. + * The value 'N' can be used as the end of a range to indicate the maximum + * allowed value; i.e (nmaskbits - 1). * * Returns: 0 on success, -errno on invalid input strings. Error values: * @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) if (buf == NULL) return 0; - buf = bitmap_parse_region(buf, &r); + buf = bitmap_parse_region(buf, &r, nmaskbits); if (IS_ERR(buf)) return PTR_ERR(buf); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist() 2021-01-21 22:33 ` [PATCH 3/3] lib: support N as end of range in bitmap_parselist() Paul Gortmaker @ 2021-01-22 0:29 ` Yury Norov 2021-01-22 4:43 ` Paul Gortmaker 0 siblings, 1 reply; 10+ messages in thread From: Yury Norov @ 2021-01-22 0:29 UTC (permalink / raw) To: Paul Gortmaker Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > > While this is done for all bitmaps, the original use case in mind was > for CPU masks and cpulist_parse(). Credit to Yury who suggested to > push it down from CPU subsys to bitmap - it simplified things a lot. Can you convert your credit to Suggested-by or Reviewed-by? :) > It seems that a common configuration is to use the 1st couple cores > for housekeeping tasks, and or driving a busy peripheral that generates > a lot of interrupts, or something similar. > > This tends to leave the remaining ones to form a pool of similarly > configured cores to take on the real workload of interest to the user. > > So on machine A - with 32 cores, it could be 0-3 for "system" and then > 4-31 being used in boot args like nohz_full=, or rcu_nocbs= as part of > setting up the worker pool of CPUs. > > But then newer machine B is added, and it has 48 cores, and so while > the 0-3 part remains unchanged, the pool setup cpu list becomes 4-47. > > Deployment would be easier if we could just simply replace 31 and 47 > with "N" and let the system substitute in the actual number at boot; > a number that it knows better than we do. > > No need to have custom boot args per node, no need to do a trial boot > in order to snoop /proc/cpuinfo and/or /sys/devices/system/cpu - no > more fencepost errors of using 32 and 48 instead of 31 and 47. > > Cc: Yury Norov <yury.norov@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> > --- > .../admin-guide/kernel-parameters.rst | 4 ++++ > lib/bitmap.c | 18 +++++++++++++----- > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst > index 5e080080b058..668f0b69fb4f 100644 > --- a/Documentation/admin-guide/kernel-parameters.rst > +++ b/Documentation/admin-guide/kernel-parameters.rst > @@ -68,6 +68,10 @@ For example one can add to the command line following parameter: > > where the final item represents CPUs 100,101,125,126,150,151,... > > +The value "N" can be used as the end of a range, to represent the numerically > +last CPU on the system, i.e "foo_cpus=16-N" would be equivalent to "16-31" on > +a 32 core system. > + > The following convenience aliases are also accepted and used: > > foo_cpus=all > diff --git a/lib/bitmap.c b/lib/bitmap.c > index a1010646fbe5..d498ea9d526b 100644 > --- a/lib/bitmap.c > +++ b/lib/bitmap.c > @@ -571,7 +571,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end > return end; > } > > -static const char *bitmap_parse_region(const char *str, struct region *r) > +static const char *bitmap_parse_region(const char *str, struct region *r, int nmaskbits) > { in bitmap_parselist() you can store nmaskbits in the struct region, and avoid passing nmaskbits as a parameter. > str = bitmap_getnum(str, &r->start); > if (IS_ERR(str)) > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > if (*str != '-') > return ERR_PTR(-EINVAL); > > - str = bitmap_getnum(str + 1, &r->end); > - if (IS_ERR(str)) > - return str; > + str++; > + if (*str == 'N') { > + r->end = nmaskbits - 1; > + str++; > + } else { > + str = bitmap_getnum(str, &r->end); > + if (IS_ERR(str)) > + return str; > + } Indeed it's much simpler. But I don't like that you increase the nesting level. Can you keep bitmap_parse_region() a single-tab style function? What about group size? Are you going to support N there, like "0-N:5/N"? What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()? I would also like to see tests covering new functionality. As a user of "N", I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including error codes that the parser returns. Otherwise it will be hard to maintain the transition. > if (end_of_region(*str)) > goto no_pattern; > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > * Syntax: range:used_size/group_size > * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 > * Optionally the self-descriptive "all" or "none" can be used. > + * The value 'N' can be used as the end of a range to indicate the maximum > + * allowed value; i.e (nmaskbits - 1). > * > * Returns: 0 on success, -errno on invalid input strings. Error values: > * > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > if (buf == NULL) > return 0; > > - buf = bitmap_parse_region(buf, &r); > + buf = bitmap_parse_region(buf, &r, nmaskbits); > if (IS_ERR(buf)) > return PTR_ERR(buf); > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist() 2021-01-22 0:29 ` Yury Norov @ 2021-01-22 4:43 ` Paul Gortmaker 2021-01-22 23:08 ` Yury Norov 0 siblings, 1 reply; 10+ messages in thread From: Paul Gortmaker @ 2021-01-22 4:43 UTC (permalink / raw) To: Yury Norov; +Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote: > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker > <paul.gortmaker@windriver.com> wrote: > > > > While this is done for all bitmaps, the original use case in mind was > > for CPU masks and cpulist_parse(). Credit to Yury who suggested to > > push it down from CPU subsys to bitmap - it simplified things a lot. > > Can you convert your credit to Suggested-by or Reviewed-by? :) Sure, of course. [...] > > diff --git a/lib/bitmap.c b/lib/bitmap.c > > index a1010646fbe5..d498ea9d526b 100644 > > --- a/lib/bitmap.c > > +++ b/lib/bitmap.c > > @@ -571,7 +571,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end > > return end; > > } > > > > -static const char *bitmap_parse_region(const char *str, struct region *r) > > +static const char *bitmap_parse_region(const char *str, struct region *r, int nmaskbits) > > { > > in bitmap_parselist() you can store nmaskbits in the struct region, and avoid > passing nmaskbits as a parameter. OK. FWIW, I considered that and went with the param so as to not open the door to someone possibly using an uninitialized struct value later. > > str = bitmap_getnum(str, &r->start); > > if (IS_ERR(str)) > > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > if (*str != '-') > > return ERR_PTR(-EINVAL); > > > > - str = bitmap_getnum(str + 1, &r->end); > > - if (IS_ERR(str)) > > - return str; > > + str++; > > + if (*str == 'N') { > > + r->end = nmaskbits - 1; > > + str++; > > + } else { > > + str = bitmap_getnum(str, &r->end); > > + if (IS_ERR(str)) > > + return str; > > + } > > Indeed it's much simpler. But I don't like that you increase the nesting level. > Can you keep bitmap_parse_region() a single-tab style function? Rather a strict coding style, but we can replace with: if (*str == 'N') { r->end = nmaskbits - 1; str++; } else { str = bitmap_getnum(str, &r->end); } if (IS_ERR(str)) return str; Is that what you were after? > What about group size? Are you going to support N there, like "0-N:5/N"? No. I would think that the group size has to be less than 1/2 of the nmaskbits or you get the rather pointless case of just one group. Plus conflating "end of range" with "group size" just adds confusion. So it is currently not legal: root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 4-5,8-9,12-13 root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# > What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()? The "N-N" is also not supported/legal. The allowed use is listed as being for the end of a range only. The code enforces this by ensuring the char previous is a '-' ; hence a leading N is invalid: root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus /bin/echo: write error: Invalid argument root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 0-15 root@hackbox:/sys/fs/cgroup/cpuset/foo# I think "use for end of range only" makes sense in the mathematical sense most of us have seen during school: {0, 1, 2, ... N-1, N} as used in the end point of a range of numbers. I could make the "only" part more explicit and concrete in the comments/docs if desired. I'm not sure I see the value in complicating things in order to add or extend support to non-intuitive use cases beyond that - to me that seems to just make things more confusing for end users. But again if you've something in mind that I'm simply missing, then by all means please elaborate. > I would also like to see tests covering new functionality. As a user of "N", > I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including > error codes that the parser returns. Otherwise it will be hard to maintain the > transition. That is a reasonable request. I will look into adding "N" based type tests to the existing bitmap test cases in a separate commit. Thanks, Paul. -- > > > if (end_of_region(*str)) > > goto no_pattern; > > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > * Syntax: range:used_size/group_size > > * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 > > * Optionally the self-descriptive "all" or "none" can be used. > > + * The value 'N' can be used as the end of a range to indicate the maximum > > + * allowed value; i.e (nmaskbits - 1). > > * > > * Returns: 0 on success, -errno on invalid input strings. Error values: > > * > > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > > if (buf == NULL) > > return 0; > > > > - buf = bitmap_parse_region(buf, &r); > > + buf = bitmap_parse_region(buf, &r, nmaskbits); > > if (IS_ERR(buf)) > > return PTR_ERR(buf); > > > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist() 2021-01-22 4:43 ` Paul Gortmaker @ 2021-01-22 23:08 ` Yury Norov 2021-01-26 17:18 ` Paul Gortmaker 0 siblings, 1 reply; 10+ messages in thread From: Yury Norov @ 2021-01-22 23:08 UTC (permalink / raw) To: Paul Gortmaker Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney On Thu, Jan 21, 2021 at 8:44 PM Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > > [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote: > > > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker > > <paul.gortmaker@windriver.com> wrote: > > > > > > While this is done for all bitmaps, the original use case in mind was > > > for CPU masks and cpulist_parse(). Credit to Yury who suggested to > > > push it down from CPU subsys to bitmap - it simplified things a lot. > > > > Can you convert your credit to Suggested-by or Reviewed-by? :) > > Sure, of course. > > [...] > > > > diff --git a/lib/bitmap.c b/lib/bitmap.c > > > index a1010646fbe5..d498ea9d526b 100644 > > > --- a/lib/bitmap.c > > > +++ b/lib/bitmap.c > > > @@ -571,7 +571,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end > > > return end; > > > } > > > > > > -static const char *bitmap_parse_region(const char *str, struct region *r) > > > +static const char *bitmap_parse_region(const char *str, struct region *r, int nmaskbits) > > > { > > > > in bitmap_parselist() you can store nmaskbits in the struct region, and avoid > > passing nmaskbits as a parameter. > > OK. FWIW, I considered that and went with the param so as to not open > the door to someone possibly using an uninitialized struct value later. > > > > str = bitmap_getnum(str, &r->start); > > > if (IS_ERR(str)) > > > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > > if (*str != '-') > > > return ERR_PTR(-EINVAL); > > > > > > - str = bitmap_getnum(str + 1, &r->end); > > > - if (IS_ERR(str)) > > > - return str; > > > + str++; > > > + if (*str == 'N') { > > > + r->end = nmaskbits - 1; > > > + str++; > > > + } else { > > > + str = bitmap_getnum(str, &r->end); > > > + if (IS_ERR(str)) > > > + return str; > > > + } > > > > Indeed it's much simpler. But I don't like that you increase the nesting level. > > Can you keep bitmap_parse_region() a single-tab style function? > > Rather a strict coding style, but we can replace with: > > if (*str == 'N') { > r->end = nmaskbits - 1; > str++; > } else { > str = bitmap_getnum(str, &r->end); > } > > if (IS_ERR(str)) > return str; > > Is that what you were after? > > > What about group size? Are you going to support N there, like "0-N:5/N"? > > No. I would think that the group size has to be less than 1/2 of > the nmaskbits or you get the rather pointless case of just one group. > Plus conflating "end of range" with "group size" just adds confusion. > So it is currently not legal: > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 4-5,8-9,12-13 > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus > /bin/echo: write error: Invalid argument > root@hackbox:/sys/fs/cgroup/cpuset/foo# > > > What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()? > > The "N-N" is also not supported/legal. The allowed use is listed as > being for the end of a range only. The code enforces this by ensuring > the char previous is a '-' ; hence a leading N is invalid: > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus > /bin/echo: write error: Invalid argument > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 0-15 > root@hackbox:/sys/fs/cgroup/cpuset/foo# > > I think "use for end of range only" makes sense in the mathematical > sense most of us have seen during school: {0, 1, 2, ... N-1, N} as > used in the end point of a range of numbers. I could make the "only" > part more explicit and concrete in the comments/docs if desired. > > I'm not sure I see the value in complicating things in order to add > or extend support to non-intuitive use cases beyond that - to me that > seems to just make things more confusing for end users. But again > if you've something in mind that I'm simply missing, then by all > means please elaborate. OK, let me share my view on this. As you said in the patch description, N is substitution to the number of the last CPU, in your example sort of #define N (15). So, when I do echo N-N > cpuset.cpus, I want it to work as if I do echo 15-15 > cpuset.cpus. Why? Because in my terribly huge config I just want to do s/15/N. Now let's check how it works . root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 15-15>cpuset.cpus root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 15 root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-0>cpuset.cpus root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 0 OK, works as expected. And what about N? > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 0-15 OK, looks good. > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus > /bin/echo: write error: Invalid argument Why? If N is 15, it should work exactly as 15-15, but it doesn't... This is the source of confusion and unneeded refactoring of user scripts. (In practice of course nobody will use "N" because it's broken.) Documentation says nothing about this limitation, and this is a real example of "complicating things". You can do better - parse "N" in bitmap_getnum() and avoid all that confusion. Same logic regarding all/none: all is the equivalent of 0-15, none is something like ", ,". Now let's try with "0-15,0-3, ," (imagine it's a result of merging configs). root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-15,0-3, , >cpuset.cpus root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus 0-15 OK, works. But if I do 's/0-15/all' and 's/ /none', things get broken. This again brings a special case where it can be avoided - just parse all/none at the beginning of bitmap_parse_region(). After reading the description, one can think that you introduce simple and convenient extensions to existing interface. In fact this is a new interface which is mostly incompatible with the existing one. > > I would also like to see tests covering new functionality. As a user of "N", > > I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including > > error codes that the parser returns. Otherwise it will be hard to maintain the > > transition. > > That is a reasonable request. I will look into adding "N" based type > tests to the existing bitmap test cases in a separate commit. > > Thanks, > Paul. > -- > > > > > > if (end_of_region(*str)) > > > goto no_pattern; > > > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > > * Syntax: range:used_size/group_size > > > * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 > > > * Optionally the self-descriptive "all" or "none" can be used. > > > + * The value 'N' can be used as the end of a range to indicate the maximum > > > + * allowed value; i.e (nmaskbits - 1). > > > * > > > * Returns: 0 on success, -errno on invalid input strings. Error values: > > > * > > > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > > > if (buf == NULL) > > > return 0; > > > > > > - buf = bitmap_parse_region(buf, &r); > > > + buf = bitmap_parse_region(buf, &r, nmaskbits); > > > if (IS_ERR(buf)) > > > return PTR_ERR(buf); > > > > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist() 2021-01-22 23:08 ` Yury Norov @ 2021-01-26 17:18 ` Paul Gortmaker 0 siblings, 0 replies; 10+ messages in thread From: Paul Gortmaker @ 2021-01-26 17:18 UTC (permalink / raw) To: Yury Norov; +Cc: Linux Kernel Mailing List, Peter Zijlstra, Paul E. McKenney [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 22/01/2021 (Fri 15:08) Yury Norov wrote: > On Thu, Jan 21, 2021 at 8:44 PM Paul Gortmaker > <paul.gortmaker@windriver.com> wrote: > > > > [Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()] On 21/01/2021 (Thu 16:29) Yury Norov wrote: > > > > > On Thu, Jan 21, 2021 at 2:34 PM Paul Gortmaker > > > <paul.gortmaker@windriver.com> wrote: > > > > > > > > While this is done for all bitmaps, the original use case in mind was > > > > for CPU masks and cpulist_parse(). Credit to Yury who suggested to > > > > push it down from CPU subsys to bitmap - it simplified things a lot. > > > > > > Can you convert your credit to Suggested-by or Reviewed-by? :) > > > > Sure, of course. Now done for v3. > > > > [...] > > > > > > diff --git a/lib/bitmap.c b/lib/bitmap.c > > > > index a1010646fbe5..d498ea9d526b 100644 > > > > --- a/lib/bitmap.c > > > > +++ b/lib/bitmap.c > > > > @@ -571,7 +571,7 @@ static const char *bitmap_find_region_reverse(const char *start, const char *end > > > > return end; > > > > } > > > > > > > > -static const char *bitmap_parse_region(const char *str, struct region *r) > > > > +static const char *bitmap_parse_region(const char *str, struct region *r, int nmaskbits) > > > > { > > > > > > in bitmap_parselist() you can store nmaskbits in the struct region, and avoid > > > passing nmaskbits as a parameter. > > > > OK. FWIW, I considered that and went with the param so as to not open > > the door to someone possibly using an uninitialized struct value later. Also now done - reduces parameter passing and enables moving a sanity check from set_region into check_region where it IMHO belongs. > > > > > > str = bitmap_getnum(str, &r->start); > > > > if (IS_ERR(str)) > > > > @@ -583,9 +583,15 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > > > if (*str != '-') > > > > return ERR_PTR(-EINVAL); > > > > > > > > - str = bitmap_getnum(str + 1, &r->end); > > > > - if (IS_ERR(str)) > > > > - return str; > > > > + str++; > > > > + if (*str == 'N') { > > > > + r->end = nmaskbits - 1; > > > > + str++; > > > > + } else { > > > > + str = bitmap_getnum(str, &r->end); > > > > + if (IS_ERR(str)) > > > > + return str; > > > > + } > > > > > > Indeed it's much simpler. But I don't like that you increase the nesting level. > > > Can you keep bitmap_parse_region() a single-tab style function? No increase in nesting level in v3. > > > > Rather a strict coding style, but we can replace with: > > > > if (*str == 'N') { > > r->end = nmaskbits - 1; > > str++; > > } else { > > str = bitmap_getnum(str, &r->end); > > } > > > > if (IS_ERR(str)) > > return str; > > > > Is that what you were after? > > > > > What about group size? Are you going to support N there, like "0-N:5/N"? > > > > No. I would think that the group size has to be less than 1/2 of > > the nmaskbits or you get the rather pointless case of just one group. > > Plus conflating "end of range" with "group size" just adds confusion. > > So it is currently not legal: > > > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus > > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > > 4-5,8-9,12-13 > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus > > /bin/echo: write error: Invalid argument > > root@hackbox:/sys/fs/cgroup/cpuset/foo# > > > > > What about "N-N"? Is it legal? Maybe hide new logic in bitmap_getnum()? > > > > The "N-N" is also not supported/legal. The allowed use is listed as > > being for the end of a range only. The code enforces this by ensuring > > the char previous is a '-' ; hence a leading N is invalid: > > > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus > > /bin/echo: write error: Invalid argument > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus > > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > > 0-15 > > root@hackbox:/sys/fs/cgroup/cpuset/foo# > > > > I think "use for end of range only" makes sense in the mathematical > > sense most of us have seen during school: {0, 1, 2, ... N-1, N} as > > used in the end point of a range of numbers. I could make the "only" > > part more explicit and concrete in the comments/docs if desired. > > > > I'm not sure I see the value in complicating things in order to add > > or extend support to non-intuitive use cases beyond that - to me that > > seems to just make things more confusing for end users. But again > > if you've something in mind that I'm simply missing, then by all > > means please elaborate. > > OK, let me share my view on this. As you said in the patch description, > N is substitution to the number of the last CPU, in your example sort of > #define N (15). > > So, when I do echo N-N > cpuset.cpus, I want it to work as if I do > echo 15-15 > cpuset.cpus. Why? Because in my terribly huge config > I just want to do s/15/N. Okay, That might seem like a simple point, but it helped a lot in letting me understand how you were looking at this and in turn what kinds of "interesting" parameter combinations are currently accepted. > > Now let's check how it works . > > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 15-15>cpuset.cpus > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 15 > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-0>cpuset.cpus > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 0 > > OK, works as expected. And what about N? > > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus > > root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > > 0-15 > > OK, looks good. > > > root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus > > /bin/echo: write error: Invalid argument > > Why? If N is 15, it should work exactly as 15-15, but it doesn't... > This is the source > of confusion and unneeded refactoring of user scripts. (In practice of course > nobody will use "N" because it's broken.) Documentation says nothing about > this limitation, and this is a real example of "complicating things". So the disconnect was that I was thinking "where does N make sense to use" and you were thinking "N is just a number - let it be used anywhere and let the sanity checks take care of the rest". I can see that now. I have added self-tests for some of these interesting (but yet valid) inputs, like "15-15:15/15" as a way to document what is currently supported. I then copied those (i.e "N-N:N/N") to ensure that N is consistent with being "just a number" as you outlined above. > You can do better - parse "N" in bitmap_getnum() and avoid all that confusion. N is now handled in getnum in v3 of the series and treated as simply just another number, with no special cases related to where it appears. > Same logic regarding all/none: all is the equivalent of 0-15, none is something > like ", ,". Now let's try with "0-15,0-3, ," (imagine it's a result of > merging configs). > > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-15,0-3, , >cpuset.cpus > root@yury-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus > 0-15 Given that "none" has no users (now or envisioned) and basically amounts to being a no-op as ", ," it seems to make sense to just drop it. Which made me reconsider implementing "all" as well. Since "0-N" is the same strlen as "all" and achieves exactly the same result, I've dropped the idea of accepting "all" as well in the interest of keeping it simple. Please have a look at v3 if you have the time and see if there are any remaining things you'd like to see implemented differently, or things I've inadvertently missed. There are more commits (added tests) but each change is hopefully a small, easy to review, standalone item. It seems like vger/lkml is having some issues, but assuming the mails eventually get through, the v3 series should get archived at: https://lore.kernel.org/lkml/20210126171141.122639-1-paul.gortmaker@windriver.com/ Thanks, Paul. -- > > OK, works. But if I do 's/0-15/all' and 's/ /none', things get broken. This > again brings a special case where it can be avoided - just parse all/none > at the beginning of bitmap_parse_region(). > > After reading the description, one can think that you introduce simple and > convenient extensions to existing interface. In fact this is a new interface > which is mostly incompatible with the existing one. > > > > I would also like to see tests covering new functionality. As a user of "N", > > > I want to be 100% sure that this "N" is a full equivalent of NR_CPUS, including > > > error codes that the parser returns. Otherwise it will be hard to maintain the > > > transition. > > > > That is a reasonable request. I will look into adding "N" based type > > tests to the existing bitmap test cases in a separate commit. > > > > Thanks, > > Paul. > > -- > > > > > > > > > if (end_of_region(*str)) > > > > goto no_pattern; > > > > @@ -628,6 +634,8 @@ static const char *bitmap_parse_region(const char *str, struct region *r) > > > > * Syntax: range:used_size/group_size > > > > * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769 > > > > * Optionally the self-descriptive "all" or "none" can be used. > > > > + * The value 'N' can be used as the end of a range to indicate the maximum > > > > + * allowed value; i.e (nmaskbits - 1). > > > > * > > > > * Returns: 0 on success, -errno on invalid input strings. Error values: > > > > * > > > > @@ -656,7 +664,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits) > > > > if (buf == NULL) > > > > return 0; > > > > > > > > - buf = bitmap_parse_region(buf, &r); > > > > + buf = bitmap_parse_region(buf, &r, nmaskbits); > > > > if (IS_ERR(buf)) > > > > return PTR_ERR(buf); > > > > > > > > -- > > > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-27 0:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 22:33 [PATCH v2 0/3] :support for bitmap (and hence CPU) list abbreviations Paul Gortmaker 2021-01-21 22:33 ` [PATCH 1/3] lib: add "all" and "none" as valid ranges to bitmap_parselist() Paul Gortmaker 2021-01-22 0:07 ` Yury Norov 2021-01-22 4:34 ` Paul Gortmaker 2021-01-21 22:33 ` [PATCH 2/3] rcu: dont special case "all" handling; let bitmask deal with it Paul Gortmaker 2021-01-21 22:33 ` [PATCH 3/3] lib: support N as end of range in bitmap_parselist() Paul Gortmaker 2021-01-22 0:29 ` Yury Norov 2021-01-22 4:43 ` Paul Gortmaker 2021-01-22 23:08 ` Yury Norov 2021-01-26 17:18 ` Paul Gortmaker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).