Message-ID: <20210122044357.GS16838@windriver.com>
Date:   Thu, 21 Jan 2021 23:43:58 -0500
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH 3/3] lib: support N as end of range in bitmap_parselist()

[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@...driver.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@...kbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/4 > cpuset.cpus
root@...kbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
4-5,8-9,12-13
root@...kbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 4-N:2/N > cpuset.cpus
/bin/echo: write error: Invalid argument
root@...kbox:/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@...kbox:/sys/fs/cgroup/cpuset/foo# /bin/echo N-N > cpuset.cpus
/bin/echo: write error: Invalid argument
root@...kbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 0-N > cpuset.cpus
root@...kbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
0-15
root@...kbox:/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
> >

Powered by blists - more mailing lists