[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.gortma...@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 > >