From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dario Faggioli <dario.faggioli@citrix.com>
Subject: Re: [RFC PATCH v4] xen: credit2: provide custom option
 to create runqueue
Date: Fri, 9 Jun 2017 18:41:48 +0200
Message-ID: <1497026508.26212.13.camel@citrix.com>
References: <20170419174518.986-1-kpraveen.lkml@gmail.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3988002167212811646=="
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20170419174518.986-1-kpraveen.lkml@gmail.com>
List-Unsubscribe: <https://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <https://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Errors-To: xen-devel-bounces@lists.xen.org
Sender: "Xen-devel" <xen-devel-bounces@lists.xen.org>
To: Praveen Kumar <kpraveen.lkml@gmail.com>, george.dunlap@eu.citrix.com
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

--===============3988002167212811646==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="=-/mTbr/F70XjW7z0rlQHJ"

--=-/mTbr/F70XjW7z0rlQHJ
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hey Praveen,

Here we are, sorry for the delay.

On Wed, 2017-04-19 at 23:15 +0530, Praveen Kumar wrote:
> index 33e54aef63..f2ee4ad972 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -525,7 +525,7 @@ also slow in responding to load changes.
> =C2=A0The default value of `1 sec` is rather long.
> =C2=A0
> =C2=A0### credit2\_runqueue
> -> `=3D cpu | core | socket | node | all`
> +> `=3D cpu | core | socket | node | all | <custom>`
> =C2=A0
> =C2=A0> Default: `socket`
> =C2=A0
> @@ -543,6 +543,14 @@ Available alternatives, with their meaning, are:
> =C2=A0* `node`: one runqueue per each NUMA node of the host;
> =C2=A0* `all`: just one runqueue shared by all the logical pCPUs of
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0the host
> +* `<custom>`: one runqueue per mentioned subset. The subset can be
> defined as
>
Make the lines shorter than 80 characters.

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0as shown in below example:
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0credit2_runqueue=3D[[0,1,][2,6][3,5][4,7]] , or
> 0,1\;2,6\;3,5\;4,7
>
Here as well. Also, what's the purpose of the '\'? Why are they there?

If it's because of GRUB (well, at least, that's what I used, and I
indeed have seen it interfering, when it sees a ';'), I think it's
enough to use:

 credit2_runqueue=3D'0,1;2,6;3,5;4,7'

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0which means :
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- pCPUs 0 and 1 belong to runqueue 0
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- pCPUs 2 and 6 belong to runqueue 1
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- pCPUs 3 and 5 belong to runqueue 2
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0- pCPUs 4 and 7 belong to runqueue 3
> =C2=A0
> =C2=A0### dbgp
> =C2=A0> `=3D ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index b9b928347f..ebec33f450 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -321,6 +321,16 @@ integer_param("credit2_balance_over",
> opt_overload_balance_tolerance);
> =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0(logical) processors of the host belong. This will
> happen if
> =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0the opt_runqueue parameter is set to 'all'.
> =C2=A0 *
> + * - custom: meaning that there will be one runqueue per subset
> being passed as
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0para=
meter to credit2_runqueue as shown in below
> example.
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Exam=
ple:
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cred=
it2_runqueue=3D[[cpu0,cpu1][cpu3][cpu4,cpu5]] or
>
Why 'cpu0', 'cpu1', etc? Let's use numbers, like everywhere else ('0',
'1', etc).

> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cred=
it2_runqueue=3D0,1\;3\;4,5
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0The =
example mentioned states :
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0cpu0 and cpu1 belongs to runqueue 0
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0cpu3 belongs to runqueue 1
> + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0cpu4 and cpu5 belongs to runqueue 2
> + *
>
I also think that, both here and above (i.e., in xen-command-
line.markdonw), it's worth quickly mentioning what happens to pCPUs
that don't appear in the list.

And, about this, see my other comments, down in this mail, because I
think we should change that.

> @@ -330,15 +340,138 @@ integer_param("credit2_balance_over",
> opt_overload_balance_tolerance);
> =C2=A0#define OPT_RUNQUEUE_SOCKET 2
> =C2=A0#define OPT_RUNQUEUE_NODE=C2=A0=C2=A0=C2=A03
> =C2=A0#define OPT_RUNQUEUE_ALL=C2=A0=C2=A0=C2=A0=C2=A04
> +#define OPT_RUNQUEUE_CUSTOM 5
> =C2=A0static const char *const opt_runqueue_str[] =3D {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_CPU] =3D "cpu",
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_CORE] =3D "core",
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_SOCKET] =3D "socket",
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_NODE] =3D "node",
> -=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_ALL] =3D "all"
> +=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_ALL] =3D "all",
> +=C2=A0=C2=A0=C2=A0=C2=A0[OPT_RUNQUEUE_CUSTOM] =3D "custom"
>
Why does this need to be in the array? We don't expect to ever find
something like:

 credit2_runqueue=3Dcustom

(and, if we do, it's wrong!)

> =C2=A0};
> =C2=A0static int __read_mostly opt_runqueue =3D OPT_RUNQUEUE_SOCKET;
> =C2=A0
> +static unsigned long __read_mostly custom_cpu_runqueue[NR_CPUS];
> +
As I've said in my previous review, this should be allocated
dynamically, and be no larger than nr_cpu_ids.

> +static inline int getlen(const char *start, const char *end)
> +{
> +=C2=A0=C2=A0=C2=A0=C2=A0if ( ( start ) && ( end ) && ( end > start ) )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return end-start;
> +=C2=A0=C2=A0=C2=A0=C2=A0else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return -1;
> +}
> +
There should be no need for anything like this.

> +static int parse_custom_runqueue_option(const char *s)
>
I think 'parse_custom_runqueue' is enough (not a big deal, of course,
but I like it better).

Also, we're interested in returning whether the parsing succeeded or
failed right? Then, the return type should be bool.

> +{
> +=C2=A0=C2=A0=C2=A0=C2=A0const char *parse =3D NULL, *s_end =3D NULL;
> +=C2=A0=C2=A0=C2=A0=C2=A0const char *start =3D NULL, *end =3D NULL;
> +=C2=A0=C2=A0=C2=A0=C2=A0char delimiter[2] =3D {0};
> +=C2=A0=C2=A0=C2=A0=C2=A0int cpu_added_to_runqueue =3D 0;
> +=C2=A0=C2=A0=C2=A0=C2=A0int runqueue =3D 0;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0/* Format supported :
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,=
15]]
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0or
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* 0,1,4,5\;2,3,6,7\;8,9,12,13\;10,11,14,15
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/
> +=C2=A0=C2=A0=C2=A0=C2=A0parse =3D s;
>
You can just use s as iterator.

> +=C2=A0=C2=A0=C2=A0=C2=A0s_end =3D s + strlen(s);
>
What's s_end needed for?

For instance, this...

> +=C2=A0=C2=A0=C2=A0=C2=A0/* The start and should always be in format of '=
[..]' */
> +=C2=A0=C2=A0=C2=A0=C2=A0if ( ( '[' =3D=3D *parse ) && ( ']' =3D=3D *(s_e=
nd-1)) )
>
can be:

 if ( parse[0] =3D=3D '[' && parse(strlen(parse)] =3D=3D ']' )

> +=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0delimiter[0] =3D '[';
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0delimiter[1] =3D '\0';
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0parse++;
> +=C2=A0=C2=A0=C2=A0=C2=A0}
> +=C2=A0=C2=A0=C2=A0=C2=A0else
> +=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0delimiter[0] =3D ';';
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0delimiter[1] =3D '\0';
> +=C2=A0=C2=A0=C2=A0=C2=A0}
> +
Mmm... and what if parse[0] is ']', but parse[strlen(parse)] is not
']'. That would happen, if we are dealing with a malformed string, such
as "[[0,1][2," (or something like that). In that case, what we want is
to exit the function, and declare the custom parsing failed.

So, I think we need something like this:

if ( parse[0] =3D=3D '[' && parse[strlen(parse)] =3D=3D ']' )
{
    //here, we know we're dealing with format one,
    //i.e., [[0,1][3,5]]
    ...
}
else if ( isdigit(parse[0]) && isdigit(parse[strlen(parse)]) )
{
  =C2=A0=C2=A0//here, we know we're dealing with format two,
  =C2=A0=C2=A0//i.e., 0,1;3,5
  =C2=A0=C2=A0...
}
else
{
    //everything that is different than the two cases above,
    //is a malformed option string.
    return false;
}

Also, what's the point of having delimiter[1], if it's always equal to
'\0'. At minimum, if we really need it, let's assign '\0' to it outside
of the if.

> +=C2=A0=C2=A0=C2=A0=C2=A0while ( ( parse !=3D NULL ) && ( parse < s_end )=
 )
>
why not just `while ( *parse !=3D '\0' )` ?

> +=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const char *token_sub_st=
r =3D NULL;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0while ( *parse =3D=3D '[=
' )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
parse++;
> +
Why while? This makes me thing that we accept something like
"[[[[[[[0,1][[[0,3]]" (or maybe even worse formatted variants, such as
"[[[0,[[1[[,2]]")!

Shouldn't this actually check the opposite, i.e., that, if we are on a
'[', then the next character needs to be a number (since we only want
one '[' for opening a subset, and we don't accept empty subsets)?

Such as, for instance:

  /*
   * We want only numbers at the beginning of a subset,
   * and we don't support empty subsets.
   */
  if ( (*parse =3D=3D '[' && !isdigit(*(parse+1))) || /* Format 1 */=20
       (*parse =3D=3D ';' && !isdigit(*(parse+1))) )  /* Format 2 */
      return false;

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0start =3D parse;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0end =3D strstr(parse, de=
limiter);
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Check if we don't hav=
e the delimiter */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( !end )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* If we don't have delimiter, then break, if start is
> greater than
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0* or equal to s_end, as we have reached the end.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0*/
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( start >=3D s_end )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0break;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* We need to parse till s_end, as we have the last set
> */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
end =3D s_end;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> +
>
I don't understand what you're doing here. Why do you want to know
about all this now? Just go ahead parsing.

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Just move to next, as=
 we have empty set like [] or ;; */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( getlen ( start, end=
 ) < 1 )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
goto next;
> +
Oh, you wanted to support empty support empty subsets. Well, I don't
particularly mind, but I think it's easier if we forbid them.

If you instead feel strong about wanting to tolerate them, then the
'if' I suggested above should be tweaked a little.

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/*
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* find token withi=
n the subset
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
unsigned long token =3D 0;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* Get cpu ids separated by ',' within each set */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
token_sub_str =3D strpbrk(start, ",");
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* Basic checks to validate the last entry in subset */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( ( !token_sub_str && start < end ) ||
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0( token_sub_str > end && token_sub_str > start ) )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if ( ( delimiter[0] =3D=3D '[' ) && ( start =3D=3D =
s_end - 1
> ) )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto next;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0token_sub_str =3D end;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
}
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* Just move to next, as we have empty set like [] or ;;
> */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( getlen(start, token_sub_str) < 1 )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0goto next;
> +
Sorry, I'm lost again about why we need this inner loop for processing
substrings, searching for tokens, etc.

I'll try to state more clearly how I think this can be made in a way
simpler way. Just one thing first, about simple_strtoul...

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
token =3D simple_strtoul(start ,&token_sub_str, 0);
> +
...here, we do know in what base we want the numbers we accept to be,
so let's use 10, instead than 0.

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( token >=3D nr_cpu_ids)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return -1;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* If not set already */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( custom_cpu_runqueue[token] =3D=3D -1 )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0custom_cpu_runqueue[token] =3D runqueue;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0cpu_added_to_runqueue =3D 1;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
}
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return -1;
>
Here and everywhere else, `return false`.
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
if ( !token_sub_str || token_sub_str > end )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0goto next;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
start =3D ++token_sub_str;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} while ( start < end );
> +next:
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( cpu_added_to_runque=
ue )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
runqueue++;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
cpu_added_to_runqueue =3D 0;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0parse =3D ++end;
> +=C2=A0=C2=A0=C2=A0=C2=A0}
> +=C2=A0=C2=A0=C2=A0=C2=A0opt_runqueue =3D OPT_RUNQUEUE_CUSTOM;
>
This should not be done here. It should be done in
parse_credit2_runqueue() iff calling parse_custom_runqueue() succeeds.

> +=C2=A0=C2=A0=C2=A0=C2=A0return 0;
>
`return true`

> +}
> +

Anyway, as I've said a couple of times already,=C2=A0things can be
simplified a lot, without any need for any nested loop, or complex
substring manipulation.

Here's how I'd do it:

    if ( *s =3D=3D '[' && *(s+strlen(s)-1) =3D=3D ']' )
        s++;
    else if ( !(isdigit(*s) && isdigit(s[strlen(s)-1])) )
        return false;

    while ( !(*s =3D=3D ']' && *(s+1) =3D=3D '\0') && *s !=3D '\0' )
    {
        /*
         * We tolerate only the allowed characters (depending on the
         * format). Also, we don't accept empty subsets.
         */
        if ( *(s+strlen(s)-1) =3D=3D ']' )
        {
            /* Format 1 */
            if ( !(*s =3D=3D '[' || *s =3D=3D ']' || *s =3D=3D ',' || isdig=
it(*s)) ||
                 (*s =3D=3D '[' && *(s+1) =3D=3D ']') )
                return false;
        }
        else
        {
            /* Format 2 */
            if ( !(*s =3D=3D ';' || *s =3D=3D ',' || isdigit(*s)) ||
                 (*s =3D=3D ';' && *(s+1) =3D=3D ';') )
                return false;
        }

        /* Are we at the beginning of a subset, in format 1? */
        if ( *s =3D=3D '[' )
        {
            s++;
            in_subset =3D true;
            continue;
        }
        /* Are we at the end of a subset? */
        if ( *s =3D=3D ']' || *s =3D=3D ';' )
        {
            /*
             * Well, if we are closing a subset, we must be inside
             * one. If not, the string is malformed.
             */
            if ( *s =3D=3D ']' && !in_subset )
                return false;

            s++;
            rqi++;
            in_subset =3D false;
            continue;
        }

        /*
         * At this point, we must be dealing with either a number
         * (the ID of a CPU) or the within subset separator (a ',').
         */
        if ( *s =3D=3D ',' )
        {
            s++;
            continue;
        }
        cpu =3D simple_strtoul(s, &s, 10);

        /* Is the cpu ID we found valid? */
        if ( cpu >=3D nr_cpu_ids )
            return false;
        custom_cpu_runqueue[cpu] =3D rqi;
    }

This is just an example, and I've only quickly tested that it builds,
and that it deals with well formed strings, and a couple of bad formed
ones.

If you adopt it (or some variation of it), please run proper stress
testing with multiple examples of malformed input.

Anyway, as you can see, it's just one pass, there's no nested loops at
all, and there's no need to keep track of delimiters, or to deal with
substrings.

> @@ -351,8 +484,29 @@ static void parse_credit2_runqueue(const char
> *s)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0return;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> +=C2=A0=C2=A0=C2=A0=C2=A0/*
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* At this stage we are either unknown valu=
e of credit2_runqueue
> or we can
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* consider it to be custom cpu. Lets try p=
arsing the same.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Resetting the custom_cpu_runqueue for fu=
ture use. Only the
> non-negative
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* entries will be valid. The index 'i' in =
custom_cpu_runqueue
> will store
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* the specific runqueue it belongs to.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Example:
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0If custom_c=
pu_runqueue[3] =3D=3D 2
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Then, it me=
ans that cpu 3 belong to runqueue 2.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0If custom_c=
pu_runqueue[4] =3D=3D -1
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Then, it me=
ans that cpu 4 doesn't belong to any runqueue.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/
> +=C2=A0=C2=A0=C2=A0=C2=A0for ( i =3D 0; i < nr_cpu_ids; i++ )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0custom_cpu_runqueue[i] =
=3D -1;
>=20
So, as I said in my previous review, I think that having CPUs that are
under the control of a Credit2 instance, but are not part of any
runqueue, requires more changes.

I think it would be ok to just assume that any CPU present in the host,
that is not mentioned in the custom runqueue string, is assigned to
runqueue by default.

This of course should be documented in.

With a static custom_cpu_runqueue array, as you have now, that would
happen by default, by just leaving it alone (it's in the BSS, and hence
will contain all zeros).

But since I'm asking you to dynamically allocate it, that would mean
you'll have to use xzalloc() (or xzalloc_array()).
=C2=A0
> -=C2=A0=C2=A0=C2=A0=C2=A0printk("WARNING, unrecognized value of credit2_r=
unqueue
> option!\n");
> +=C2=A0=C2=A0=C2=A0=C2=A0if ( parse_custom_runqueue_option(s) !=3D 0 )
> +=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Resetting in case of =
failure, so that we don't mess-up
> during any failure
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* due to wrong or =
spurious pattern passed by user.
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0opt_runqueue =3D OPT_RUN=
QUEUE_SOCKET;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0printk("WARNING, unrecog=
nized value of credit2_runqueue
> option!\n");
>
If you do as I said above, and don't set opt_runqueue to CUSTOM if the
parsing fails, you then don't have to reset it here.

> +=C2=A0=C2=A0=C2=A0=C2=A0}
> =C2=A0}
> =C2=A0custom_param("credit2_runqueue", parse_credit2_runqueue);
> =C2=A0
> @@ -662,6 +816,15 @@ cpu_to_runqueue(struct csched2_private *prv,
> unsigned int cpu)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct csched2_runqueue_data *rqd;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int rqi;
> =C2=A0
> +=C2=A0=C2=A0=C2=A0=C2=A0if ( opt_runqueue =3D=3D OPT_RUNQUEUE_CUSTOM )
> +=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( custom_cpu_runqueue=
[cpu] !=3D -1 )
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
BUG_ON(custom_cpu_runqueue[cpu] >=3D nr_cpu_ids);
>
We should check this during parsing, and either fail or ignore that
CPU, rather than dying in here.

> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
return custom_cpu_runqueue[cpu];
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> +=C2=A0=C2=A0=C2=A0=C2=A0}
> +
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for ( rqi =3D 0; rqi < nr_cpu_ids; rqi++ )
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int peer_c=
pu;

Regards,
Dario
--=20
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--=-/mTbr/F70XjW7z0rlQHJ
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJZOs/OAAoJEBZCeImluHPuAFMQAM5sakYcXLyNv8ZbA4Zo3iOR
0fFvDLIuRJNPhSGZMTsxewFkuENXE10kLzucLYEwuE22KYj55F1VoNzzuGObMyEu
LMbYN05zXhq52uiN+FWI1Svn9/u1ux7dZXeCQwqgcAsOr50v3H6DowcgB47BY5gu
FAsTyIwP8kY0afz0p/8LGvJr0nvdPvlKeKCZ5MLSu14dnG2CqVJ2foSzGoR1vm58
yBqNsKghDqiF94bAU+2iwL4DK1T35g0lqeksODzdM9bdetAYvPy8SKcAv5lU5XlD
MtLMEk/wv48hKYY0Q+Hq0n6/RM436Sf4IxHGmTWuQroDPldTtpBvpP8Ampnj+a8s
Uty2JasWOP5KxiStd0W1jcfWJd03iTj//FElmiOnb0AnZdtseKEq+t7/eOMwRdGS
4XJixH/OUdLzRF6VoWHr4Df3j/mzUNQ8jOhtGB6cy4xvO7QeQRMFpigGcuZSKBRR
YXWUMowHbPCpW9k0qlDa7SeqABsUP9ucRGvlgKYe4wWzZuJUvjCkjLBxaAJu5x78
GzTiE7oFnELRYANwVytP7XfDZbbd5ha2/iH4zKM4LJotGgmquHUs8Z+zV9u4xWSL
QSOWb4PiB8B2dl+jgbM0Jwrjcb/rKe/va6JWDDZGUmikQYZS/puaz+9cRF887CWT
Y/eoI1MItWzUdZD0LBQc
=aop0
-----END PGP SIGNATURE-----

--=-/mTbr/F70XjW7z0rlQHJ--


--===============3988002167212811646==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs
IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v
cmcveGVuLWRldmVsCg==

--===============3988002167212811646==--