Message-ID: <20210126171811.GC23530@windriver.com>
Date:   Tue, 26 Jan 2021 12:18:12 -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 22/01/2021 (Fri 15:08) Yury Norov wrote:

> On Thu, Jan 21, 2021 at 8:44 PM Paul Gortmaker
> <paul.gortmaker@...driver.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@...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.

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@...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.
> 
> 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@...y-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 15-15>cpuset.cpus
> root@...y-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 15
> root@...y-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-0>cpuset.cpus
> root@...y-ThinkPad:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus
> 0
> 
> OK, works as expected. And what about N?
> 
> > 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
> 
> OK, looks good.
> 
> > root@...kbox:/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@...y-ThinkPad:/sys/fs/cgroup/cpuset/foo# echo 0-15,0-3, , >cpuset.cpus
> root@...y-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
> > > >

Powered by blists - more mailing lists