|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v4] xen: credit2: provide custom option to create runqueue
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.
> The default value of `1 sec` is rather long.
>
> ### credit2\_runqueue
> -> `= cpu | core | socket | node | all`
> +> `= cpu | core | socket | node | all | <custom>`
>
> > Default: `socket`
>
> @@ -543,6 +543,14 @@ Available alternatives, with their meaning, are:
> * `node`: one runqueue per each NUMA node of the host;
> * `all`: just one runqueue shared by all the logical pCPUs of
> the host
> +* `<custom>`: one runqueue per mentioned subset. The subset can be
> defined as
>
Make the lines shorter than 80 characters.
> + as shown in below example:
> + credit2_runqueue=[[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='0,1;2,6;3,5;4,7'
> + which means :
> + - pCPUs 0 and 1 belong to runqueue 0
> + - pCPUs 2 and 6 belong to runqueue 1
> + - pCPUs 3 and 5 belong to runqueue 2
> + - pCPUs 4 and 7 belong to runqueue 3
>
> ### dbgp
> > `= 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);
> * (logical) processors of the host belong. This will
> happen if
> * the opt_runqueue parameter is set to 'all'.
> *
> + * - custom: meaning that there will be one runqueue per subset
> being passed as
> + * parameter to credit2_runqueue as shown in below
> example.
> + * Example:
> + * credit2_runqueue=[[cpu0,cpu1][cpu3][cpu4,cpu5]] or
>
Why 'cpu0', 'cpu1', etc? Let's use numbers, like everywhere else ('0',
'1', etc).
> + * credit2_runqueue=0,1\;3\;4,5
> + * The example mentioned states :
> + * cpu0 and cpu1 belongs to runqueue 0
> + * cpu3 belongs to runqueue 1
> + * cpu4 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);
> #define OPT_RUNQUEUE_SOCKET 2
> #define OPT_RUNQUEUE_NODE 3
> #define OPT_RUNQUEUE_ALL 4
> +#define OPT_RUNQUEUE_CUSTOM 5
> static const char *const opt_runqueue_str[] = {
> [OPT_RUNQUEUE_CPU] = "cpu",
> [OPT_RUNQUEUE_CORE] = "core",
> [OPT_RUNQUEUE_SOCKET] = "socket",
> [OPT_RUNQUEUE_NODE] = "node",
> - [OPT_RUNQUEUE_ALL] = "all"
> + [OPT_RUNQUEUE_ALL] = "all",
> + [OPT_RUNQUEUE_CUSTOM] = "custom"
>
Why does this need to be in the array? We don't expect to ever find
something like:
credit2_runqueue=custom
(and, if we do, it's wrong!)
> };
> static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
>
> +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)
> +{
> + if ( ( start ) && ( end ) && ( end > start ) )
> + return end-start;
> + else
> + return -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.
> +{
> + const char *parse = NULL, *s_end = NULL;
> + const char *start = NULL, *end = NULL;
> + char delimiter[2] = {0};
> + int cpu_added_to_runqueue = 0;
> + int runqueue = 0;
> +
> + /* Format supported :
> + * [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
> + * or
> + * 0,1,4,5\;2,3,6,7\;8,9,12,13\;10,11,14,15
> + */
> + parse = s;
>
You can just use s as iterator.
> + s_end = s + strlen(s);
>
What's s_end needed for?
For instance, this...
> + /* The start and should always be in format of '[..]' */
> + if ( ( '[' == *parse ) && ( ']' == *(s_end-1)) )
>
can be:
if ( parse[0] == '[' && parse(strlen(parse)] == ']' )
> + {
> + delimiter[0] = '[';
> + delimiter[1] = '\0';
> + parse++;
> + }
> + else
> + {
> + delimiter[0] = ';';
> + delimiter[1] = '\0';
> + }
> +
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] == '[' && parse[strlen(parse)] == ']' )
{
//here, we know we're dealing with format one,
//i.e., [[0,1][3,5]]
...
}
else if ( isdigit(parse[0]) && isdigit(parse[strlen(parse)]) )
{
//here, we know we're dealing with format two,
//i.e., 0,1;3,5
...
}
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.
> + while ( ( parse != NULL ) && ( parse < s_end ) )
>
why not just `while ( *parse != '\0' )` ?
> + {
> + const char *token_sub_str = NULL;
> +
> + while ( *parse == '[' )
> + 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 == '[' && !isdigit(*(parse+1))) || /* Format 1 */
(*parse == ';' && !isdigit(*(parse+1))) ) /* Format 2 */
return false;
> + start = parse;
> + end = strstr(parse, delimiter);
> +
> + /* Check if we don't have the delimiter */
> + if ( !end )
> + {
> + /* If we don't have delimiter, then break, if start is
> greater than
> + * or equal to s_end, as we have reached the end.
> + */
> + if ( start >= s_end )
> + break;
> +
> + /* We need to parse till s_end, as we have the last set
> */
> + end = s_end;
> + }
> +
>
I don't understand what you're doing here. Why do you want to know
about all this now? Just go ahead parsing.
> + /* Just move to next, as we have empty set like [] or ;; */
> + if ( getlen ( start, end ) < 1 )
> + 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.
> + /*
> + * find token within the subset
> + */
> + do
> + {
> + unsigned long token = 0;
> +
> + /* Get cpu ids separated by ',' within each set */
> + token_sub_str = strpbrk(start, ",");
> +
> + /* Basic checks to validate the last entry in subset */
> + if ( ( !token_sub_str && start < end ) ||
> + ( token_sub_str > end && token_sub_str > start ) )
> + {
> + if ( ( delimiter[0] == '[' ) && ( start == s_end - 1
> ) )
> + goto next;
> +
> + token_sub_str = end;
> + }
> +
> + /* Just move to next, as we have empty set like [] or ;;
> */
> + if ( getlen(start, token_sub_str) < 1 )
> + goto 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...
> + token = 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.
> + if ( token >= nr_cpu_ids)
> + return -1;
> +
> + /* If not set already */
> + if ( custom_cpu_runqueue[token] == -1 )
> + {
> + custom_cpu_runqueue[token] = runqueue;
> + cpu_added_to_runqueue = 1;
> + }
> + else
> + return -1;
>
Here and everywhere else, `return false`.
> +
> + if ( !token_sub_str || token_sub_str > end )
> + goto next;
> +
> + start = ++token_sub_str;
> + } while ( start < end );
> +next:
> + if ( cpu_added_to_runqueue )
> + {
> + runqueue++;
> + cpu_added_to_runqueue = 0;
> + }
> +
> + parse = ++end;
> + }
> + opt_runqueue = OPT_RUNQUEUE_CUSTOM;
>
This should not be done here. It should be done in
parse_credit2_runqueue() iff calling parse_custom_runqueue() succeeds.
> + return 0;
>
`return true`
> +}
> +
Anyway, as I've said a couple of times already, things 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 == '[' && *(s+strlen(s)-1) == ']' )
s++;
else if ( !(isdigit(*s) && isdigit(s[strlen(s)-1])) )
return false;
while ( !(*s == ']' && *(s+1) == '\0') && *s != '\0' )
{
/*
* We tolerate only the allowed characters (depending on the
* format). Also, we don't accept empty subsets.
*/
if ( *(s+strlen(s)-1) == ']' )
{
/* Format 1 */
if ( !(*s == '[' || *s == ']' || *s == ',' || isdigit(*s)) ||
(*s == '[' && *(s+1) == ']') )
return false;
}
else
{
/* Format 2 */
if ( !(*s == ';' || *s == ',' || isdigit(*s)) ||
(*s == ';' && *(s+1) == ';') )
return false;
}
/* Are we at the beginning of a subset, in format 1? */
if ( *s == '[' )
{
s++;
in_subset = true;
continue;
}
/* Are we at the end of a subset? */
if ( *s == ']' || *s == ';' )
{
/*
* Well, if we are closing a subset, we must be inside
* one. If not, the string is malformed.
*/
if ( *s == ']' && !in_subset )
return false;
s++;
rqi++;
in_subset = 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 == ',' )
{
s++;
continue;
}
cpu = simple_strtoul(s, &s, 10);
/* Is the cpu ID we found valid? */
if ( cpu >= nr_cpu_ids )
return false;
custom_cpu_runqueue[cpu] = 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)
> return;
> }
> }
> + /*
> + * At this stage we are either unknown value of credit2_runqueue
> or we can
> + * consider it to be custom cpu. Lets try parsing the same.
> + * Resetting the custom_cpu_runqueue for future use. Only the
> non-negative
> + * entries will be valid. The index 'i' in custom_cpu_runqueue
> will store
> + * the specific runqueue it belongs to.
> + * Example:
> + * If custom_cpu_runqueue[3] == 2
> + * Then, it means that cpu 3 belong to runqueue 2.
> + * If custom_cpu_runqueue[4] == -1
> + * Then, it means that cpu 4 doesn't belong to any runqueue.
> + */
> + for ( i = 0; i < nr_cpu_ids; i++ )
> + custom_cpu_runqueue[i] = -1;
>
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()).
> - printk("WARNING, unrecognized value of credit2_runqueue
> option!\n");
> + if ( parse_custom_runqueue_option(s) != 0 )
> + {
> + /* Resetting in case of failure, so that we don't mess-up
> during any failure
> + * due to wrong or spurious pattern passed by user.
> + */
> + opt_runqueue = OPT_RUNQUEUE_SOCKET;
> + printk("WARNING, unrecognized 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.
> + }
> }
> custom_param("credit2_runqueue", parse_credit2_runqueue);
>
> @@ -662,6 +816,15 @@ cpu_to_runqueue(struct csched2_private *prv,
> unsigned int cpu)
> struct csched2_runqueue_data *rqd;
> unsigned int rqi;
>
> + if ( opt_runqueue == OPT_RUNQUEUE_CUSTOM )
> + {
> + if ( custom_cpu_runqueue[cpu] != -1 )
> + {
> + BUG_ON(custom_cpu_runqueue[cpu] >= nr_cpu_ids);
>
We should check this during parsing, and either fail or ignore that
CPU, rather than dying in here.
> + return custom_cpu_runqueue[cpu];
> + }
> + }
> +
> for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
> {
> unsigned int peer_cpu;
Regards,
Dario
--
<<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)Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |