[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.