Re: [Xen-devel] [PATCH v3 40/47] xen/sched: prepare per-cpupool scheduling granularity

["Jürgen Groß" <jgross@suse.com>]

On 24.09.19 15:34, Jan Beulich wrote:
> On 14.09.2019 10:52, Juergen Gross wrote:
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -175,6 +175,8 @@ static struct cpupool *cpupool_create(
>>               return NULL;
>>           }
>>       }
>> +    c->granularity = sched_granularity;
>> +    c->opt_granularity = opt_sched_granularity;
>>   
>>       *q = c;
>>   
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index e5b7678dc0..b3c1aa0821 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -56,7 +56,8 @@ int sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>>   integer_param("sched_ratelimit_us", sched_ratelimit_us);
>>   
>>   /* Number of vcpus per struct sched_unit. */
>> -static unsigned int __read_mostly sched_granularity = 1;
>> +enum sched_gran __read_mostly opt_sched_granularity = SCHED_GRAN_cpu;
>> +unsigned int __read_mostly sched_granularity = 1;
> 
> Seeing the replacements you do further down, are these variables
> needed at all anymore outside of cpupool.c? If not, they should
> probably move there, and remain / become static?

Hmm, good idea.

> 
>> --- a/xen/include/xen/sched-if.h
>> +++ b/xen/include/xen/sched-if.h
>> @@ -25,6 +25,15 @@ extern int sched_ratelimit_us;
>>   /* Scheduling resource mask. */
>>   extern const cpumask_t *sched_res_mask;
>>   
>> +/* Number of vcpus per struct sched_unit. */
>> +enum sched_gran {
>> +    SCHED_GRAN_cpu,
>> +    SCHED_GRAN_core,
>> +    SCHED_GRAN_socket
>> +};
> 
> Seeing the almost absurd arrangement on my AMD Fam17 system (128 CPUs
> per Credit2 runqueue, for a total of two runqueues) I really wonder
> whether there shouldn't be a plan for a further intermediate
> granularity between "core" and "socket". The other day I did notice
> Linux has gained the concept of "die", which would bring the
> arrangement to a more reasonable 8 runqueues of 32 CPUs each on this
> system. (I'm taking Credit2 as a reference here only.)

Okay, another item for "scheduler cleanup" I guess.

> 
>> @@ -532,6 +542,8 @@ struct cpupool
>>       struct cpupool   *next;
>>       struct scheduler *sched;
>>       atomic_t         refcnt;
>> +    unsigned int     granularity;
>> +    enum sched_gran  opt_granularity;
> 
> I'd like to suggest to avoid introducing opt_* identifiers not
> directly driven by command line options. That'll just end up
> confusing people. I have to admit I'm having trouble coming up with
> good names for both fields, so I'd like to ask whether you really
> need both - isn't what's called "granularity" above a function of
> "opt_granularity"?

Only indirectly. I need opt_granularity for selecting the correct
cpumask (cpumask_of(), cpu_sibling_mask(), ...). granularity is the
numerical value which I don't want to calculate each time I need it.

> Or alternatively couldn't what's named
> "granularity" now be always taken from struct sched_resource? I
> take it that within a pool all struct sched_resource instances
> would have the same numeric granularity value. And I further take
> it that struct sched_resource instances won't freely move between
> cpupools (and hence could e.g. be associated with their pool on
> linked list, the head of which lives in struct cpupool).

I think I wouldn't need such a linked list. All cases where I need
cpupool->granularity are not performance critical, so I could easily
calculate it from cpupool->opt_granularity or by using
cpupool->res_valid for finding a sched_resource of that cpupool.

I'll rename cpupool->opt_granularity to gran and drop
cpupool->granularity.


Juergen

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