|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v3 2/2] xen: credit2: provide custom option to create
On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> The patch introduces a new command line option 'custom' that when
used will
> create runqueue based upon the pCPU subset provide during bootup.
>
"introduces a new, very flexible, way of arranging runqueues in
Credit2. It allows to specify, explicitly and precisely, what pCPUs
should belong to which runqueue"
Or something like this.
It's great that you've got the code working. I have some comments,
though, on how it actually looks like.
> Signed-off-by: Praveen Kumar <kpraveen.lkml@xxxxxxxxx>
> ---
> --- 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`
>
Err... but this is not really correct, right?
I mean, it's not that the user should use the word 'custom' here.
You probably want to use something like <custom>, and then define what
that means below.
> @@ -543,6 +543,12 @@ 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 subset. Example:
> + credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14
> ,15]]
>
Maybe just use 2 (or at most 3) pCPUs per runqueue in the example. It'd
make the line shorter and easier to read and understand.
> + - pCPUs 0, 1, 4 and 5 belong to runqueue 0
> + - pCPUs 2, 3, 6 and 7 belong to runqueue 1
> + - pCPUs 8, 9, 12 and 13 belong to runqueue 2
> + - pCPUs 10, 11, 14 and 15 belong to runqueue 3
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -330,18 +339,60 @@ 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"
> };
> static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
>
> +static int __read_mostly custom_cpu_runqueue[NR_CPUS];
> +
I think this can be nr_cpu_ids (and be allocated dynamically during
parsing).
> +#define GETTOKEN( token, len, start, end ) \
> +{ \
> + char _tmp[len+1]; \
> + int _i; \
> + safe_strcpy(_tmp, start); \
> + _tmp[len] = '\0'; \
> + for ( _i = 0; _tmp[_i] != '\0'; _i++ ) \
> + token = ( ( token * 10 ) + ( _tmp[_i] - '0' ) ); \
> +}
> +
If you really need this, make it 'static inline', as you do for other
functions.
As a matter of fact, I don't think you need it, as, for instance, we do
have simple_strtoul() (and some others). :-)
> +static inline int trim(const char *c, char *t, char elem)
> +{
> + int l = strlen(c);
> + const char *x = c ;
> + int i = 0;
> + if ( !c || !t )
> + return -1;
> + while ( *x != '\0' && i < l )
> + {
> + if ( *x != elem )
> + t[i++] = *x;
> + x++;
> + }
> + t[i] = '\0';
> + return 0;
> +}
> +
Again, why we need this? Can't we just deal with invalid characters
while parsing the string (by, e.g., either skipping them or aborting,
depending on whether or not they're innocuous)?
> +static inline int getlen(char *start, char *end)
> +{
> + if ( ( start ) && ( end ) && ( end > start ) )
> + return end-start;
> + else
> + return -1;
> +}
> +
>
Same.
> static void parse_credit2_runqueue(const char *s)
> {
> unsigned int i;
> + const char *s_end = NULL;
> + char m[strlen(s)];
> + char *_s = NULL;
>
No '_' prefixed variable names, please. :-)
Also, what's m for?
> @@ -351,7 +402,115 @@ 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;
> +
> + /*
> + * Format [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
> + */
> + i = 0;
> +
> + /* In case user have spaces included in the input */
> + if ( trim(s, m, ' ') != 0 )
> + {
> + printk( "WARNING : %s[%d] trim failed.\n", __func__,
> __LINE__ );
> + goto errReturn;
> + }
>
Right. So, as I was saying above, can't we just start parsing the
string --with something like a `char *p; while ( *p != '\0' ) {...}`--
and skip the spaces when we find them?
> + /* Starting to parse and get the cpu information on trimmed data
> */
> + _s = m;
> + s_end = _s + strlen(_s);
> +
> + /* The start and should always be in format of '[..]' */
> + if ( ( '[' == *_s ) && ( ']' == *(s_end-1)) )
>
But in the cover you said we also support:
credit2_runqueue=0,1,4,5;2,3,6,7;8,9,12,13;10,11,14,15
So, what I think we want here is something that checks that, if the
first character is '[', then the last must be ']', or we already know
the format is invalid, and we can stop.
And it would also be better to write an if that checks whether the
format is _invalid_, and stop the parsing if that is the case. That
saves a level of indentation for all the code that follows.
> + {
> + char *start = NULL, *end = NULL;
> + int cpu_added_to_runqueue = 0;
>
It looks like this can be bool.
> + _s++;
> + while ( ( _s != NULL ) && ( _s < s_end ) )
> + {
> + char *token_sub_str = NULL;
>
We want a blank line between variable definition and actual code.
> + start = strstr(_s, "[");
> + end = strstr(_s, "]");
> + /* Validation checks for '[' and ']' to properly parse
> the entries
> + */
> + if ( ( !start && !end ) || ( ( end == ( s_end -1 ) ) &&
> start ) )
> + goto errReturn;
>
No camel case either, please. 'err' is probably just fine. :-)
But, honestly, I can't quite tell what's going on here. Why strstr()?
You can just check whether the first char of a set is '[' by looking at
the current string parsing cursor. If it is, you can take a note about
that somewhere, to remember to check for a ']' at some point.
> + /* Check for last entry */
> + else if ( !start && ( end == ( s_end -1 ) ) )
> + goto nextSet;
> +
> + /* If we have [] as entry, move to nextSet */
> + if ( getlen ( start, end ) < 1 )
> + goto nextSet;
>
'next' (or 'next_set', but I think 'next' is fine).
> + /* Start to parse the actual value */
> + start++;
> + /*
> + * find token within the subset
> + */
> + do
> + {
> + int token = 0;
> + int len = 0;
> + /* Get cpu ids separated by ',' within each set */
> + token_sub_str = strpbrk(start, ",");
> + if ( ( !token_sub_str && start < end ) ||
> + ( token_sub_str > end && token_sub_str > start )
> )
> + len = getlen(start, end);
> + else
> + len = getlen(start, token_sub_str);
> +
> + if ( len <= 0 )
> + continue;
> + /* GETTOKEN will get return the parse and populate
> the cpu in
> + * token
> + */
> + GETTOKEN(token, len, start , end );
> +
> + if ( token >= nr_cpu_ids)
> + goto errReturn;
> + /* If not set already */
> + if ( custom_cpu_runqueue[token] == -1 )
> + {
> + custom_cpu_runqueue[token] = i;
> + cpu_added_to_runqueue = 1;
> + }
> + else
> + goto errReturn;
> +
> + if ( !token_sub_str || token_sub_str > end )
> + goto nextSet;
> +
> + start = ++token_sub_str;
> + } while ( start < end );
> +nextSet:
> + if ( cpu_added_to_runqueue )
> + {
> + i++;
> + cpu_added_to_runqueue = 0;
> + }
>
>
> + _s = ++end;
> + }
>
Ok, so this looks more complex that it could be, to me. Mostly because
you're doing your own token parsing.
In my mind, parsing a string like the ones we want to accept, should
look not too different from what's done inside init_boot_pages().
Indeed our case looks a bit more complicated, but the general idea
should be the same, i.e.:
- scan the string and deal with what you find in there online, not
beforehand;
- use simple_strtoul()
> @@ -661,6 +820,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);
> + return custom_cpu_runqueue[cpu];
> + }
>
Wait, and then what happens if a CPU was not in any set, and hence has
it's custom_cpu_runqueue element == -1 ?
I am ok with it not being added to any runqueue, but for that to be
what really happens, I think we need to change code in init_pdata()
too.
It would be also good to print a warning, in such case. This is a
custom mode, and the user may well be doing such a thing for a reason,
but it doesn't harm trying make sure nothing this bad happened by
mistake.
Thanks again for all this work! :-D
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 |