| Message ID | 20201201082128.15239-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
On 01.12.2020 09:21, Juergen Gross wrote: > @@ -260,23 +257,42 @@ static struct cpupool *cpupool_create( > > spin_lock(&cpupool_lock); > > - for_each_cpupool(q) > + if ( poolid != CPUPOOLID_NONE ) > { > - last = (*q)->cpupool_id; > - if ( (poolid != CPUPOOLID_NONE) && (last >= poolid) ) > - break; > + q = __cpupool_find_by_id(poolid, false); > + if ( !q ) > + list_add_tail(&c->list, &cpupool_list); > + else > + { > + list_add_tail(&c->list, &q->list); > + if ( q->cpupool_id == poolid ) > + { > + *perr = -EEXIST; > + goto err; > + } You bail _after_ having added the new entry to the list? > + } > + > + c->cpupool_id = poolid; > } > - if ( *q != NULL ) > + else > { > - if ( (*q)->cpupool_id == poolid ) > + /* Cpupool 0 is created with specified id at boot and never removed. */ > + ASSERT(!list_empty(&cpupool_list)); > + > + q = list_last_entry(&cpupool_list, struct cpupool, list); > + /* In case of wrap search for first free id. */ > + if ( q->cpupool_id == CPUPOOLID_NONE - 1 ) > { > - *perr = -EEXIST; > - goto err; > + list_for_each_entry(q, &cpupool_list, list) > + if ( q->cpupool_id + 1 != list_next_entry(q, list)->cpupool_id ) > + break; > } > - c->next = *q; > + > + list_add(&c->list, &q->list); > + > + c->cpupool_id = q->cpupool_id + 1; What guarantees that you managed to find an unused ID, other than at current CPU speeds it taking too long to create 4 billion pools? Since you're doing this under lock, wouldn't it help anyway to have a global helper variable pointing at the lowest pool followed by an unused ID? Jan
On 01.12.20 10:12, Jan Beulich wrote: > On 01.12.2020 09:21, Juergen Gross wrote: >> @@ -260,23 +257,42 @@ static struct cpupool *cpupool_create( >> >> spin_lock(&cpupool_lock); >> >> - for_each_cpupool(q) >> + if ( poolid != CPUPOOLID_NONE ) >> { >> - last = (*q)->cpupool_id; >> - if ( (poolid != CPUPOOLID_NONE) && (last >= poolid) ) >> - break; >> + q = __cpupool_find_by_id(poolid, false); >> + if ( !q ) >> + list_add_tail(&c->list, &cpupool_list); >> + else >> + { >> + list_add_tail(&c->list, &q->list); >> + if ( q->cpupool_id == poolid ) >> + { >> + *perr = -EEXIST; >> + goto err; >> + } > > You bail _after_ having added the new entry to the list? Yes, this is making exit handling easier. > >> + } >> + >> + c->cpupool_id = poolid; >> } >> - if ( *q != NULL ) >> + else >> { >> - if ( (*q)->cpupool_id == poolid ) >> + /* Cpupool 0 is created with specified id at boot and never removed. */ >> + ASSERT(!list_empty(&cpupool_list)); >> + >> + q = list_last_entry(&cpupool_list, struct cpupool, list); >> + /* In case of wrap search for first free id. */ >> + if ( q->cpupool_id == CPUPOOLID_NONE - 1 ) >> { >> - *perr = -EEXIST; >> - goto err; >> + list_for_each_entry(q, &cpupool_list, list) >> + if ( q->cpupool_id + 1 != list_next_entry(q, list)->cpupool_id ) >> + break; >> } >> - c->next = *q; >> + >> + list_add(&c->list, &q->list); >> + >> + c->cpupool_id = q->cpupool_id + 1; > > What guarantees that you managed to find an unused ID, other > than at current CPU speeds it taking too long to create 4 > billion pools? Since you're doing this under lock, wouldn't > it help anyway to have a global helper variable pointing at > the lowest pool followed by an unused ID? An admin doing that would be quite crazy and wouldn't deserve better. For being usable a cpupool needs to have a cpu assigned to it. And I don't think we are coming even close to 4 billion supported cpus. :-) Yes, it would be possible to create 4 billion empty cpupools, but for what purpose? There are simpler ways to make the system unusable with dom0 root access. Juergen
On Tue, 2020-12-01 at 10:18 +0100, Jürgen Groß wrote: > On 01.12.20 10:12, Jan Beulich wrote: > > What guarantees that you managed to find an unused ID, other > > than at current CPU speeds it taking too long to create 4 > > billion pools? Since you're doing this under lock, wouldn't > > it help anyway to have a global helper variable pointing at > > the lowest pool followed by an unused ID? > > An admin doing that would be quite crazy and wouldn't deserve better. > > For being usable a cpupool needs to have a cpu assigned to it. And I > don't think we are coming even close to 4 billion supported cpus. :-) > > Yes, it would be possible to create 4 billion empty cpupools, but for > what purpose? There are simpler ways to make the system unusable with > dom0 root access. > Yes, I agree. I don't think it's worth going through too much effort for trying to deal with that. What I'd do is: - add a comment here, explaining quickly exactly this fact, i.e., that it's not that we've forgotten to deal with this and it's all on purpose. Actually, it can be either a comment here or it can be mentioned in the changelog. I'm fine either way - if we're concerned about someone doing: for i=1...N { xl cpupool-create foo bar } with N ending up being some giant number, e.g., by mistake, I don't think it's unreasonable to come up with an high enough (but certainly not in the billions!) MAX_CPUPOOLS, and stop creating new ones when we reach that level. Regards
On 04.12.20 17:13, Dario Faggioli wrote: > On Tue, 2020-12-01 at 10:18 +0100, Jürgen Groß wrote: >> On 01.12.20 10:12, Jan Beulich wrote: >>> What guarantees that you managed to find an unused ID, other >>> than at current CPU speeds it taking too long to create 4 >>> billion pools? Since you're doing this under lock, wouldn't >>> it help anyway to have a global helper variable pointing at >>> the lowest pool followed by an unused ID? >> >> An admin doing that would be quite crazy and wouldn't deserve better. >> >> For being usable a cpupool needs to have a cpu assigned to it. And I >> don't think we are coming even close to 4 billion supported cpus. :-) >> >> Yes, it would be possible to create 4 billion empty cpupools, but for >> what purpose? There are simpler ways to make the system unusable with >> dom0 root access. >> > Yes, I agree. I don't think it's worth going through too much effort > for trying to deal with that. > > What I'd do is: > - add a comment here, explaining quickly exactly this fact, i.e., > that it's not that we've forgotten to deal with this and it's all > on purpose. Actually, it can be either a comment here or it can be > mentioned in the changelog. I'm fine either way > - if we're concerned about someone doing: > for i=1...N { xl cpupool-create foo bar } > with N ending up being some giant number, e.g., by mistake, I don't > think it's unreasonable to come up with an high enough (but > certainly not in the billions!) MAX_CPUPOOLS, and stop creating new > ones when we reach that level. Do you agree that this could be another patch? I'm not introducing that (theoretical) problem here. Juergen
On Fri, 2020-12-04 at 17:16 +0100, Jürgen Groß wrote: > On 04.12.20 17:13, Dario Faggioli wrote: > > > > > > What I'd do is: > > - add a comment here, explaining quickly exactly this fact, i.e., > > that it's not that we've forgotten to deal with this and it's > > all > > on purpose. Actually, it can be either a comment here or it can > > be > > mentioned in the changelog. I'm fine either way > > - if we're concerned about someone doing: > > for i=1...N { xl cpupool-create foo bar } > > with N ending up being some giant number, e.g., by mistake, I > > don't > > think it's unreasonable to come up with an high enough (but > > certainly not in the billions!) MAX_CPUPOOLS, and stop creating > > new > > ones when we reach that level. > > Do you agree that this could be another patch? > Ah, yes, sorry, got carried away and forgot to mention that! Of course it should be in another patch... But indeed I should have stated that clearly. So, trying to do better this time round: - the comment can/should be added as part of this patch. But I'm now much more convinced that a quick mention in the changelog (still of this patch) is actually better; - any "solution" (Jan's or MAX_CPUPOOLS) should go in its own patch. > I'm not introducing that (theoretical) problem here. > Indeed. Regards
On Tue, 2020-12-01 at 09:21 +0100, Juergen Gross wrote: > Instead of open coding something like a linked list just use the > available functionality from list.h. > Yep, much better. > While adding the required new include to private.h sort the includes. > > Signed-off-by: From: Juergen Gross <jgross@suse.com> > Reviewed-by: Dario Faggioli <dfaggioli@suse.com> We've discussed about the issue of creating too many cpupools later in the thread already. If, as stated there, a comment or (much better, IMO) a mention about that in the changelog is added, the R-o-b still stands. Regards
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index 01fa71dd00..714cd47ae9 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -16,6 +16,7 @@ #include <xen/init.h> #include <xen/keyhandler.h> #include <xen/lib.h> +#include <xen/list.h> #include <xen/param.h> #include <xen/percpu.h> #include <xen/sched.h> @@ -23,13 +24,10 @@ #include "private.h" -#define for_each_cpupool(ptr) \ - for ((ptr) = &cpupool_list; *(ptr) != NULL; (ptr) = &((*(ptr))->next)) - struct cpupool *cpupool0; /* Initial cpupool with Dom0 */ cpumask_t cpupool_free_cpus; /* cpus not in any cpupool */ -static struct cpupool *cpupool_list; /* linked list, sorted by poolid */ +static LIST_HEAD(cpupool_list); /* linked list, sorted by poolid */ static int cpupool_moving_cpu = -1; static struct cpupool *cpupool_cpu_moving = NULL; @@ -189,15 +187,15 @@ static struct cpupool *alloc_cpupool_struct(void) */ static struct cpupool *__cpupool_find_by_id(unsigned int id, bool exact) { - struct cpupool **q; + struct cpupool *q; ASSERT(spin_is_locked(&cpupool_lock)); - for_each_cpupool(q) - if ( (*q)->cpupool_id >= id ) - break; + list_for_each_entry(q, &cpupool_list, list) + if ( q->cpupool_id == id || (!exact && q->cpupool_id > id) ) + return q; - return (!exact || (*q == NULL) || ((*q)->cpupool_id == id)) ? *q : NULL; + return NULL; } static struct cpupool *cpupool_find_by_id(unsigned int poolid) @@ -246,8 +244,7 @@ static struct cpupool *cpupool_create( unsigned int poolid, unsigned int sched_id, int *perr) { struct cpupool *c; - struct cpupool **q; - unsigned int last = 0; + struct cpupool *q; *perr = -ENOMEM; if ( (c = alloc_cpupool_struct()) == NULL ) @@ -260,23 +257,42 @@ static struct cpupool *cpupool_create( spin_lock(&cpupool_lock); - for_each_cpupool(q) + if ( poolid != CPUPOOLID_NONE ) { - last = (*q)->cpupool_id; - if ( (poolid != CPUPOOLID_NONE) && (last >= poolid) ) - break; + q = __cpupool_find_by_id(poolid, false); + if ( !q ) + list_add_tail(&c->list, &cpupool_list); + else + { + list_add_tail(&c->list, &q->list); + if ( q->cpupool_id == poolid ) + { + *perr = -EEXIST; + goto err; + } + } + + c->cpupool_id = poolid; } - if ( *q != NULL ) + else { - if ( (*q)->cpupool_id == poolid ) + /* Cpupool 0 is created with specified id at boot and never removed. */ + ASSERT(!list_empty(&cpupool_list)); + + q = list_last_entry(&cpupool_list, struct cpupool, list); + /* In case of wrap search for first free id. */ + if ( q->cpupool_id == CPUPOOLID_NONE - 1 ) { - *perr = -EEXIST; - goto err; + list_for_each_entry(q, &cpupool_list, list) + if ( q->cpupool_id + 1 != list_next_entry(q, list)->cpupool_id ) + break; } - c->next = *q; + + list_add(&c->list, &q->list); + + c->cpupool_id = q->cpupool_id + 1; } - c->cpupool_id = (poolid == CPUPOOLID_NONE) ? (last + 1) : poolid; if ( poolid == 0 ) { c->sched = scheduler_get_default(); @@ -291,8 +307,6 @@ static struct cpupool *cpupool_create( c->gran = opt_sched_granularity; c->sched_gran = sched_granularity; - *q = c; - spin_unlock(&cpupool_lock); debugtrace_printk("Created cpupool %u with scheduler %s (%s)\n", @@ -302,6 +316,8 @@ static struct cpupool *cpupool_create( return c; err: + list_del(&c->list); + spin_unlock(&cpupool_lock); free_cpupool_struct(c); return NULL; @@ -312,27 +328,19 @@ static struct cpupool *cpupool_create( * possible failures: * - pool still in use * - cpus still assigned to pool - * - pool not in list */ static int cpupool_destroy(struct cpupool *c) { - struct cpupool **q; - spin_lock(&cpupool_lock); - for_each_cpupool(q) - if ( *q == c ) - break; - if ( *q != c ) - { - spin_unlock(&cpupool_lock); - return -ENOENT; - } + if ( (c->n_dom != 0) || cpumask_weight(c->cpu_valid) ) { spin_unlock(&cpupool_lock); return -EBUSY; } - *q = c->next; + + list_del(&c->list); + spin_unlock(&cpupool_lock); cpupool_put(c); @@ -732,17 +740,17 @@ static int cpupool_cpu_remove_prologue(unsigned int cpu) */ static void cpupool_cpu_remove_forced(unsigned int cpu) { - struct cpupool **c; + struct cpupool *c; int ret; unsigned int master_cpu = sched_get_resource_cpu(cpu); - for_each_cpupool ( c ) + list_for_each_entry(c, &cpupool_list, list) { - if ( cpumask_test_cpu(master_cpu, (*c)->cpu_valid) ) + if ( cpumask_test_cpu(master_cpu, c->cpu_valid) ) { - ret = cpupool_unassign_cpu_start(*c, master_cpu); + ret = cpupool_unassign_cpu_start(c, master_cpu); BUG_ON(ret); - ret = cpupool_unassign_cpu_finish(*c); + ret = cpupool_unassign_cpu_finish(c); BUG_ON(ret); } } @@ -929,7 +937,7 @@ const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool) void dump_runq(unsigned char key) { s_time_t now = NOW(); - struct cpupool **c; + struct cpupool *c; spin_lock(&cpupool_lock); @@ -944,12 +952,12 @@ void dump_runq(unsigned char key) schedule_dump(NULL); } - for_each_cpupool(c) + list_for_each_entry(c, &cpupool_list, list) { - printk("Cpupool %u:\n", (*c)->cpupool_id); - printk("Cpus: %*pbl\n", CPUMASK_PR((*c)->cpu_valid)); - sched_gran_print((*c)->gran, cpupool_get_granularity(*c)); - schedule_dump(*c); + printk("Cpupool %u:\n", c->cpupool_id); + printk("Cpus: %*pbl\n", CPUMASK_PR(c->cpu_valid)); + sched_gran_print(c->gran, cpupool_get_granularity(c)); + schedule_dump(c); } spin_unlock(&cpupool_lock); diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h index e69d9be1e8..6953cefa6e 100644 --- a/xen/common/sched/private.h +++ b/xen/common/sched/private.h @@ -8,8 +8,9 @@ #ifndef __XEN_SCHED_IF_H__ #define __XEN_SCHED_IF_H__ -#include <xen/percpu.h> #include <xen/err.h> +#include <xen/list.h> +#include <xen/percpu.h> #include <xen/rcupdate.h> /* cpus currently in no cpupool */ @@ -510,6 +511,7 @@ struct cpupool unsigned int n_dom; cpumask_var_t cpu_valid; /* all cpus assigned to pool */ cpumask_var_t res_valid; /* all scheduling resources of pool */ + struct list_head list; struct cpupool *next; struct scheduler *sched; atomic_t refcnt;