Re: [PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Luca Fancellu]

> On 15 Mar 2022, at 17:45, Andrew Cooper  wrote:
> 
> On 10/03/2022 17:10, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 10ea969c7af9..47fc856e0fe0 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct 
>> cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> unsigned int cpupool_get_id(const struct domain *d);
>> const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
>> +
>> +/*
>> + * cpupool_create_pool - Creates a cpupool
>> + * @pool_id: id of the pool to be created
>> + * @sched_id: id of the scheduler to be used for the pool
>> + *
>> + * Creates a cpupool with pool_id id.
>> + * The sched_id parameter identifies the scheduler to be used, if it is
>> + * negative, the default scheduler of Xen will be used.
>> + *
>> + * returns:
>> + * pointer to the struct cpupool just created, on success
>> + * NULL, on cpupool creation error
> 
> What makes you say this?  Your new function will fall over a NULL
> pointer before it returns one...

You are right, it’s a leftover from the v1, I will change it and review the 
code that uses it.

Cheers,
Luca

> 
> ~Andrew

Re: [PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Andrew Cooper]

On 10/03/2022 17:10, Luca Fancellu wrote:
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 10ea969c7af9..47fc856e0fe0 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct 
> cpupool *c);
>  int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>  unsigned int cpupool_get_id(const struct domain *d);
>  const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
> +
> +/*
> + * cpupool_create_pool - Creates a cpupool
> + * @pool_id: id of the pool to be created
> + * @sched_id: id of the scheduler to be used for the pool
> + *
> + * Creates a cpupool with pool_id id.
> + * The sched_id parameter identifies the scheduler to be used, if it is
> + * negative, the default scheduler of Xen will be used.
> + *
> + * returns:
> + * pointer to the struct cpupool just created, on success
> + * NULL, on cpupool creation error

What makes you say this?  Your new function will fall over a NULL
pointer before it returns one...

~Andrew

Re: [PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Juergen Gross]

On 10.03.22 18:10, Luca Fancellu wrote:

Create new public function to create cpupools, can take as parameter
the scheduler id or a negative value that means the default Xen
scheduler will be used.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- cpupool_create_pool doesn't check anymore for pool id uniqueness
   before calling cpupool_create. Modified commit message accordingly
---
  xen/common/sched/cpupool.c | 15 +++
  xen/include/xen/sched.h| 16 
  2 files changed, 31 insertions(+)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index a6da4970506a..89a891af7076 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -1219,6 +1219,21 @@ static void cpupool_hypfs_init(void)
  
  #endif /* CONFIG_HYPFS */
  
+struct cpupool *__init cpupool_create_pool(unsigned int pool_id, int sched_id)

+{
+struct cpupool *pool;
+
+if ( sched_id < 0 )
+sched_id = scheduler_get_default()->sched_id;
+
+pool = cpupool_create(pool_id, sched_id);
+
+BUG_ON(IS_ERR(pool));
+cpupool_put(pool);
+
+return pool;
+}
+
  static int __init cf_check cpupool_init(void)
  {
  unsigned int cpu;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 10ea969c7af9..47fc856e0fe0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct cpupool 
*c);
  int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
  unsigned int cpupool_get_id(const struct domain *d);
  const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
+
+/*
+ * cpupool_create_pool - Creates a cpupool
+ * @pool_id: id of the pool to be created
+ * @sched_id: id of the scheduler to be used for the pool
+ *
+ * Creates a cpupool with pool_id id.
+ * The sched_id parameter identifies the scheduler to be used, if it is
+ * negative, the default scheduler of Xen will be used.
+ *
+ * returns:
+ * pointer to the struct cpupool just created, on success
+ * NULL, on cpupool creation error


Either add a "." in the previous line, or rephrase (e.g.:
"pointer to the struct cpupool just created, or NULL in case of error"

I happened to read it as "pointer to the struct cpupool just created,
on success NULL, on cpupool creation error" first, which was weird.

With that fixed:

Reviewed-by: Juergen Gross 


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Luca Fancellu]

Create new public function to create cpupools, can take as parameter
the scheduler id or a negative value that means the default Xen
scheduler will be used.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- cpupool_create_pool doesn't check anymore for pool id uniqueness
  before calling cpupool_create. Modified commit message accordingly
---
 xen/common/sched/cpupool.c | 15 +++
 xen/include/xen/sched.h| 16 
 2 files changed, 31 insertions(+)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index a6da4970506a..89a891af7076 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -1219,6 +1219,21 @@ static void cpupool_hypfs_init(void)
 
 #endif /* CONFIG_HYPFS */
 
+struct cpupool *__init cpupool_create_pool(unsigned int pool_id, int sched_id)
+{
+struct cpupool *pool;
+
+if ( sched_id < 0 )
+sched_id = scheduler_get_default()->sched_id;
+
+pool = cpupool_create(pool_id, sched_id);
+
+BUG_ON(IS_ERR(pool));
+cpupool_put(pool);
+
+return pool;
+}
+
 static int __init cf_check cpupool_init(void)
 {
 unsigned int cpu;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 10ea969c7af9..47fc856e0fe0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct cpupool 
*c);
 int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
 unsigned int cpupool_get_id(const struct domain *d);
 const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
+
+/*
+ * cpupool_create_pool - Creates a cpupool
+ * @pool_id: id of the pool to be created
+ * @sched_id: id of the scheduler to be used for the pool
+ *
+ * Creates a cpupool with pool_id id.
+ * The sched_id parameter identifies the scheduler to be used, if it is
+ * negative, the default scheduler of Xen will be used.
+ *
+ * returns:
+ * pointer to the struct cpupool just created, on success
+ * NULL, on cpupool creation error
+ */
+struct cpupool *cpupool_create_pool(unsigned int pool_id, int sched_id);
+
 extern void cf_check dump_runq(unsigned char key);
 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
-- 
2.17.1