|
[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, Mar 31, 2017 at 10:32:09AM +0200, Dario Faggioli wrote:
> 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.
>
Sure, Will take care of the same.
> > + - 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).
>
Had to use NR_CPUS to make compiler happy with "error:Variably modified 'types'
at file scope".
> > +#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). :-)
>
True, will be replacing this with simple_strtoul()
> > +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.
>
Yes. Modifying the code in similar lines.
> > + {
> > + 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.
>
>
strstr() is actually helping in getting the subsets, from where we will further
parse the cpu's which will belong to same runqueue.
> > + /* 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()
Sure thing. Trying to use the exising APIs
>
> > @@ -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.
>
Yes, its better we show warning in case we receive -1 in init_pdata().
Working on the updated patch, will share once done with the basic unit test.
Thanks for your comments.
Regards,
~Praveen.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |