Instead of open coding something like a linked list just use the
available functionality from list.h.
The allocation of a new cpupool id is not aware of a possible wrap.
Fix that.
While adding the required new include to private.h sort the includes.
Signed-off-by: From: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
Signed-off-by: Juergen Gross <jgross@suse.com>
---
xen/common/sched/cpupool.c | 100 ++++++++++++++++++++-----------------
xen/common/sched/private.h | 4 +-
2 files changed, 57 insertions(+), 47 deletions(-)
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;
--
2.26.2
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
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
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
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
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 -- Dario Faggioli, Ph.D http://about.me/dario.faggioli Virtualization Software Engineer SUSE Labs, SUSE https://www.suse.com/ ------------------------------------------------------------------- <<This happens because _I_ choose it to happen!>> (Raistlin Majere)
© 2016 - 2026 Red Hat, Inc.