Re: [Xen-devel] [PATCH] xen: Credit2: enable fully custom runqueue arrangement

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/24/2018 11:44 AM, George Dunlap wrote:
> On 09/13/2017 05:21 PM, Dario Faggioli wrote:
>> The patch introduces yet another runqueue arrangement option
>> for Credit2. In fact, it allows the user to specify, explicitly
>> and precisely, what pCPUs should belong to which runqueue.
>>
>> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>> Signed-off-by: Praveen Kumar <kpraveen.lkml@xxxxxxxxx>
>> ---
>> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> Cc: Praveen Kumar <kpraveen.lkml@xxxxxxxxx>
>> ---
>> This is derived --although *heavily* reworked-- from what Praveen
>> has submitted before, here:
>>
>>  https://lists.xen.org/archives/html/xen-devel/2017-04/msg02402.html
>>  https://lists.xen.org/archives/html/xen-devel/2017-06/msg00201.html
>>
>> During my review of that, I suggested to make the array dynamically
>> allocated, using nr_cpu_ids as its size. Turns out, that does not
>> make much sense, as at the time the parameters are parsed, nr_cpu_ids
>> is still equal to NR_CPUS.
>> ---
>>  docs/misc/xen-command-line.markdown |   12 +++
>>  xen/common/sched_credit2.c          |  135 
>> +++++++++++++++++++++++++++++++++++
>>  2 files changed, 146 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/misc/xen-command-line.markdown 
>> b/docs/misc/xen-command-line.markdown
>> index 9797c8d..dbf5d4c 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -567,7 +567,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`
>>  
>> @@ -585,6 +585,16 @@ 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 each specified subset of logical
>> +              pCPUs of the host. Subsets are defined either as:
>> +              `[[0,1,][2,6][3,5][4,7]]`, or as:
> 
> Is the comma after the 1 there a typo, or do are you explicitly trying
> to communicate that you tolerate terminal commas?
> 
>> +              `'0,1;2,6;3,5;4,7'`. That 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
>> +              pCPUs that are present on the host, but are not
>> +              part of any subset, are assigned to runqueue 0.
>>  
>>  ### dbgp
>>  > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>> index 0f93ad5..10da084 100644
>> --- a/xen/common/sched_credit2.c
>> +++ b/xen/common/sched_credit2.c
>> @@ -12,6 +12,7 @@
>>  
>>  #include <xen/init.h>
>>  #include <xen/lib.h>
>> +#include <xen/ctype.h>
>>  #include <xen/sched.h>
>>  #include <xen/domain.h>
>>  #include <xen/delay.h>
>> @@ -323,6 +324,17 @@ 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 each specified
>> + *           subset, as shown in the following examples:
>> + *           - credit2_runqueue=[[0,1][3][4,5]]
>> + *           - credit2_runqueue='0,1;3;4,5'
>> + *           These (both) mean the following:
>> + *           - CPU 0 and CPU 1 belong to runqueue 0
>> + *           - CPU 3 belongs to runqueue 1
>> + *           - CPU 4 and CPU 5 belong to runqueue 2
>> + *           CPUs that are present on the host, but are not part of any
>> + *           defined subset, will be assigned to runqueue 0.
>> + *
>>   * Depending on the value of opt_runqueue, therefore, cpus that are part of
>>   * either the same physical core, the same physical socket, the same NUMA
>>   * node, or just all of them, will be put together to form runqueues.
>> @@ -332,6 +344,7 @@ 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",
>> @@ -341,6 +354,115 @@ static const char *const opt_runqueue_str[] = {
>>  };
>>  static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
>>  
>> +static unsigned int __read_mostly custom_cpu_runqueue[NR_CPUS];
>> +
>> +static int parse_custom_runqueue(const char *s)
>> +{
>> +    unsigned int cpu, rqi = 0;
>> +    bool in_subset = false, ret = true;
>> +    cpumask_t cpus;
>> +
>> +    /*
>> +     * If we are dealing with format 1 (i.e., [[0,1][3,5]]), first
>> +     * and last character must be '[' and ']', respectively.
>> +     *
>> +     * If we are dealing with format 2 (i.e., 0,1;3,5), first and
>> +     * last character must be numbers.
>> +     */
>> +    if ( *s == '[' && *(s+strlen(s)-1) == ']' )
>> +        s++;
>> +    else if ( !(isdigit(*s) && isdigit(s[strlen(s)-1])) )
>> +        return false;
>> +
>> +    cpumask_clear(&cpus);
>> +    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 == '[' )
>> +        {
>> +            /*
>> +             * If we are opening a subset, we must have closed all the
>> +             * previously defined ones, or the string is malformed.
>> +             */
>> +            if ( in_subset )
>> +                return false;
>> +
>> +            s++;
>> +            in_subset = true;
>> +            continue;
>> +        }
>> +        /* Are we at the end of a subset? */
>> +        if ( *s == ']' || *s == ';' )
>> +        {
>> +            /*
>> +             * If we are closing a subset, in format 1, we must have
>> +             * opened it before. 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 CPU ID (i.e.,
>> +         * a number) or CPU separator, within a subset (i.e., ',').
>> +         */
>> +        if ( *s == ',' )
>> +        {
>> +            s++;
>> +            continue;
>> +        }
>> +        cpu = simple_strtoul(s, &s, 10);
> 
> Er, sorry -- it looks like this won't handle multi-digit pcpu numbers;
> in fact, [01][23][45][67] will parse the same way as [0,1][2,3][4,5][6,7]?
> 
> Or am I missing something?

Oh, I see -- simple_strtoul() modifies s.  N/M.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel