xen/common/sched/core.c | 18 +++++++++--------- xen/common/sched/credit2.c | 4 ++-- xen/common/sysctl.c | 2 +- xen/include/xen/sched.h | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-)
Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"
The renaming s/sched_id/scheduler_id of the function defined in
'xen/common/sched/core.c' prevents any hiding of that function
by the many instances of omonymous function parameters.
Similarly, the renames
- s/ops/operations
- s/do_softirq/exec_softirq
- s/loop/it
are introduced for parameter names, to avoid any conflict
with the homonymous variable or function defined in an enclosing
scope.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/common/sched/core.c | 18 +++++++++---------
xen/common/sched/credit2.c | 4 ++--
xen/common/sysctl.c | 2 +-
xen/include/xen/sched.h | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 022f548652..e74b1208bd 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -99,13 +99,13 @@ static void sched_set_affinity(
struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
static struct sched_resource *cf_check
-sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
+sched_idle_res_pick(const struct scheduler *operations, const struct sched_unit *unit)
{
return unit->res;
}
static void *cf_check
-sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
+sched_idle_alloc_udata(const struct scheduler *operations, struct sched_unit *unit,
void *dd)
{
/* Any non-NULL pointer is fine here. */
@@ -113,12 +113,12 @@ sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
}
static void cf_check
-sched_idle_free_udata(const struct scheduler *ops, void *priv)
+sched_idle_free_udata(const struct scheduler *operations, void *priv)
{
}
static void cf_check sched_idle_schedule(
- const struct scheduler *ops, struct sched_unit *unit, s_time_t now,
+ const struct scheduler *operations, struct sched_unit *unit, s_time_t now,
bool tasklet_work_scheduled)
{
const unsigned int cpu = smp_processor_id();
@@ -2040,8 +2040,8 @@ long do_set_timer_op(s_time_t timeout)
return 0;
}
-/* sched_id - fetch ID of current scheduler */
-int sched_id(void)
+/* scheduler_id - fetch ID of current scheduler */
+int scheduler_id(void)
{
return ops.sched_id;
}
@@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
struct sched_unit *prev = vprev->sched_unit, *next;
s_time_t now;
spinlock_t *lock;
- bool do_softirq = false;
+ bool exec_softirq = false;
unsigned int cpu = smp_processor_id();
ASSERT_NOT_IN_ATOMIC();
@@ -2604,7 +2604,7 @@ static void cf_check sched_slave(void)
return;
}
- do_softirq = true;
+ exec_softirq = true;
}
if ( !prev->rendezvous_in_cnt )
@@ -2614,7 +2614,7 @@ static void cf_check sched_slave(void)
rcu_read_unlock(&sched_res_rculock);
/* Check for failed forced context switch. */
- if ( do_softirq )
+ if ( exec_softirq )
raise_softirq(SCHEDULE_SOFTIRQ);
return;
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 87a1e31ee9..aba51a7963 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
list_for_each_entry ( rqd, &prv->rql, rql )
{
struct list_head *iter, *runq = &rqd->runq;
- int loop = 0;
+ int it = 0;
/* We need the lock to scan the runqueue. */
spin_lock(&rqd->lock);
@@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
if ( svc )
{
- printk("\t%3d: ", loop++);
+ printk("\t%3d: ", it++);
csched2_dump_unit(prv, svc);
}
}
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 0cbfe8bd44..7cabfb0230 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -71,7 +71,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
case XEN_SYSCTL_sched_id:
- op->u.sched_id.sched_id = sched_id();
+ op->u.sched_id.sched_id = scheduler_id();
break;
case XEN_SYSCTL_getdomaininfolist:
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 854f3e32c0..bfe714d2e2 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -791,7 +791,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid);
void sched_destroy_domain(struct domain *d);
long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
long sched_adjust_global(struct xen_sysctl_scheduler_op *);
-int sched_id(void);
+int scheduler_id(void);
/*
* sched_get_id_by_name - retrieves a scheduler id given a scheduler name
--
2.34.1On 21.07.2023 17:31, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
>
> The renaming s/sched_id/scheduler_id of the function defined in
> 'xen/common/sched/core.c' prevents any hiding of that function
> by the many instances of omonymous function parameters.
>
> Similarly, the renames
> - s/ops/operations
> - s/do_softirq/exec_softirq
> - s/loop/it
> are introduced for parameter names, to avoid any conflict
> with the homonymous variable or function defined in an enclosing
> scope.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/common/sched/core.c | 18 +++++++++---------
> xen/common/sched/credit2.c | 4 ++--
> xen/common/sysctl.c | 2 +-
> xen/include/xen/sched.h | 2 +-
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 022f548652..e74b1208bd 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -99,13 +99,13 @@ static void sched_set_affinity(
> struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
>
> static struct sched_resource *cf_check
> -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
> +sched_idle_res_pick(const struct scheduler *operations, const struct sched_unit *unit)
> {
> return unit->res;
> }
>
> static void *cf_check
> -sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
> +sched_idle_alloc_udata(const struct scheduler *operations, struct sched_unit *unit,
> void *dd)
> {
> /* Any non-NULL pointer is fine here. */
> @@ -113,12 +113,12 @@ sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
> }
>
> static void cf_check
> -sched_idle_free_udata(const struct scheduler *ops, void *priv)
> +sched_idle_free_udata(const struct scheduler *operations, void *priv)
> {
> }
>
> static void cf_check sched_idle_schedule(
> - const struct scheduler *ops, struct sched_unit *unit, s_time_t now,
> + const struct scheduler *operations, struct sched_unit *unit, s_time_t now,
> bool tasklet_work_scheduled)
> {
> const unsigned int cpu = smp_processor_id();
These renames bring the idle scheduler out of sync with all others. I
think it wants considering to rename the file scope variable instead.
> @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
> struct sched_unit *prev = vprev->sched_unit, *next;
> s_time_t now;
> spinlock_t *lock;
> - bool do_softirq = false;
> + bool exec_softirq = false;
As an alternative to Stefano's suggestion, maybe consider "need_softirq"?
> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
> list_for_each_entry ( rqd, &prv->rql, rql )
> {
> struct list_head *iter, *runq = &rqd->runq;
> - int loop = 0;
> + int it = 0;
Instead of renaming, why can't we just drop this second variable, re-using
the outer scope one here (and at the same time doing away with a not really
appropriate use of plain "int")? (This may then want accompanying by ...
> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>
> if ( svc )
> {
> - printk("\t%3d: ", loop++);
> + printk("\t%3d: ", it++);
... switching to %3u here.)
Jan
On 24/07/23 10:26, Jan Beulich wrote:
> On 21.07.2023 17:31, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> The renaming s/sched_id/scheduler_id of the function defined in
>> 'xen/common/sched/core.c' prevents any hiding of that function
>> by the many instances of omonymous function parameters.
>>
>> Similarly, the renames
>> - s/ops/operations
>> - s/do_softirq/exec_softirq
>> - s/loop/it
>> are introduced for parameter names, to avoid any conflict
>> with the homonymous variable or function defined in an enclosing
>> scope.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/sched/core.c | 18 +++++++++---------
>> xen/common/sched/credit2.c | 4 ++--
>> xen/common/sysctl.c | 2 +-
>> xen/include/xen/sched.h | 2 +-
>> 4 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 022f548652..e74b1208bd 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -99,13 +99,13 @@ static void sched_set_affinity(
>> struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
>>
>> static struct sched_resource *cf_check
>> -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
>> +sched_idle_res_pick(const struct scheduler *operations, const struct sched_unit *unit)
>> {
>> return unit->res;
>> }
>>
>> static void *cf_check
>> -sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
>> +sched_idle_alloc_udata(const struct scheduler *operations, struct sched_unit *unit,
>> void *dd)
>> {
>> /* Any non-NULL pointer is fine here. */
>> @@ -113,12 +113,12 @@ sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
>> }
>>
>> static void cf_check
>> -sched_idle_free_udata(const struct scheduler *ops, void *priv)
>> +sched_idle_free_udata(const struct scheduler *operations, void *priv)
>> {
>> }
>>
>> static void cf_check sched_idle_schedule(
>> - const struct scheduler *ops, struct sched_unit *unit, s_time_t now,
>> + const struct scheduler *operations, struct sched_unit *unit, s_time_t now,
>> bool tasklet_work_scheduled)
>> {
>> const unsigned int cpu = smp_processor_id();
>
> These renames bring the idle scheduler out of sync with all others. I
> think it wants considering to rename the file scope variable instead.
>
That's ok, since the variable is static.
>> @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
>> struct sched_unit *prev = vprev->sched_unit, *next;
>> s_time_t now;
>> spinlock_t *lock;
>> - bool do_softirq = false;
>> + bool exec_softirq = false;
>
> As an alternative to Stefano's suggestion, maybe consider "need_softirq"?
>
I like it, especially since it's a local variable.
>> --- a/xen/common/sched/credit2.c
>> +++ b/xen/common/sched/credit2.c
>> @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
>> list_for_each_entry ( rqd, &prv->rql, rql )
>> {
>> struct list_head *iter, *runq = &rqd->runq;
>> - int loop = 0;
>> + int it = 0;
>
> Instead of renaming, why can't we just drop this second variable, re-using
> the outer scope one here (and at the same time doing away with a not really
> appropriate use of plain "int")? (This may then want accompanying by ...
>
>> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>>
>> if ( svc )
>> {
>> - printk("\t%3d: ", loop++);
>> + printk("\t%3d: ", it++);
>
> ... switching to %3u here.)
>
Good point.
I'll submit a v2 shortly with these changes.
Regards,
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
On Fri, 21 Jul 2023, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
>
> The renaming s/sched_id/scheduler_id of the function defined in
> 'xen/common/sched/core.c' prevents any hiding of that function
> by the many instances of omonymous function parameters.
>
> Similarly, the renames
> - s/ops/operations
> - s/do_softirq/exec_softirq
> - s/loop/it
> are introduced for parameter names, to avoid any conflict
> with the homonymous variable or function defined in an enclosing
> scope.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/common/sched/core.c | 18 +++++++++---------
> xen/common/sched/credit2.c | 4 ++--
> xen/common/sysctl.c | 2 +-
> xen/include/xen/sched.h | 2 +-
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 022f548652..e74b1208bd 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -99,13 +99,13 @@ static void sched_set_affinity(
> struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
>
> static struct sched_resource *cf_check
> -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
> +sched_idle_res_pick(const struct scheduler *operations, const struct sched_unit *unit)
nit: code style, now the line is over 80 chars, could be fixed on commit
> {
> return unit->res;
> }
>
> static void *cf_check
> -sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
> +sched_idle_alloc_udata(const struct scheduler *operations, struct sched_unit *unit,
> void *dd)
> {
> /* Any non-NULL pointer is fine here. */
> @@ -113,12 +113,12 @@ sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
> }
>
> static void cf_check
> -sched_idle_free_udata(const struct scheduler *ops, void *priv)
> +sched_idle_free_udata(const struct scheduler *operations, void *priv)
> {
> }
>
> static void cf_check sched_idle_schedule(
> - const struct scheduler *ops, struct sched_unit *unit, s_time_t now,
> + const struct scheduler *operations, struct sched_unit *unit, s_time_t now,
> bool tasklet_work_scheduled)
> {
> const unsigned int cpu = smp_processor_id();
> @@ -2040,8 +2040,8 @@ long do_set_timer_op(s_time_t timeout)
> return 0;
> }
>
> -/* sched_id - fetch ID of current scheduler */
> -int sched_id(void)
> +/* scheduler_id - fetch ID of current scheduler */
> +int scheduler_id(void)
> {
> return ops.sched_id;
> }
> @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
> struct sched_unit *prev = vprev->sched_unit, *next;
> s_time_t now;
> spinlock_t *lock;
> - bool do_softirq = false;
> + bool exec_softirq = false;
We don't typically use "exec" especially in the context of softirqs.
I would just change it to "softirq".
> unsigned int cpu = smp_processor_id();
>
> ASSERT_NOT_IN_ATOMIC();
> @@ -2604,7 +2604,7 @@ static void cf_check sched_slave(void)
> return;
> }
>
> - do_softirq = true;
> + exec_softirq = true;
> }
>
> if ( !prev->rendezvous_in_cnt )
> @@ -2614,7 +2614,7 @@ static void cf_check sched_slave(void)
> rcu_read_unlock(&sched_res_rculock);
>
> /* Check for failed forced context switch. */
> - if ( do_softirq )
> + if ( exec_softirq )
> raise_softirq(SCHEDULE_SOFTIRQ);
>
> return;
> diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
> index 87a1e31ee9..aba51a7963 100644
> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
> list_for_each_entry ( rqd, &prv->rql, rql )
> {
> struct list_head *iter, *runq = &rqd->runq;
> - int loop = 0;
> + int it = 0;
Nice catch! This is almost a bug fix.
> /* We need the lock to scan the runqueue. */
> spin_lock(&rqd->lock);
> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>
> if ( svc )
> {
> - printk("\t%3d: ", loop++);
> + printk("\t%3d: ", it++);
> csched2_dump_unit(prv, svc);
> }
> }
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 0cbfe8bd44..7cabfb0230 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -71,7 +71,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> break;
>
> case XEN_SYSCTL_sched_id:
> - op->u.sched_id.sched_id = sched_id();
> + op->u.sched_id.sched_id = scheduler_id();
I am confused about this one. There is no global variable or no other
global function named "sched_id". Why do we need to rename sched_id to
scheduler_id?
> break;
>
> case XEN_SYSCTL_getdomaininfolist:
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 854f3e32c0..bfe714d2e2 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -791,7 +791,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid);
> void sched_destroy_domain(struct domain *d);
> long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
> long sched_adjust_global(struct xen_sysctl_scheduler_op *);
> -int sched_id(void);
> +int scheduler_id(void);
>
> /*
> * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
> --
> 2.34.1
>
On 22/07/23 01:06, Stefano Stabellini wrote:
> On Fri, 21 Jul 2023, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> The renaming s/sched_id/scheduler_id of the function defined in
>> 'xen/common/sched/core.c' prevents any hiding of that function
>> by the many instances of omonymous function parameters.
>>
>> Similarly, the renames
>> - s/ops/operations
>> - s/do_softirq/exec_softirq
>> - s/loop/it
>> are introduced for parameter names, to avoid any conflict
>> with the homonymous variable or function defined in an enclosing
>> scope.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/sched/core.c | 18 +++++++++---------
>> xen/common/sched/credit2.c | 4 ++--
>> xen/common/sysctl.c | 2 +-
>> xen/include/xen/sched.h | 2 +-
>> 4 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 022f548652..e74b1208bd 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -99,13 +99,13 @@ static void sched_set_affinity(
>> struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
>>
>> static struct sched_resource *cf_check
>> -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
>> +sched_idle_res_pick(const struct scheduler *operations, const struct sched_unit *unit)
>
> nit: code style, now the line is over 80 chars, could be fixed on commit
>
Ok
>> -/* sched_id - fetch ID of current scheduler */
>> -int sched_id(void)
>> +/* scheduler_id - fetch ID of current scheduler */
>> +int scheduler_id(void)
>> {
>> return ops.sched_id;
>> }
>> @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
>> struct sched_unit *prev = vprev->sched_unit, *next;
>> s_time_t now;
>> spinlock_t *lock;
>> - bool do_softirq = false;
>> + bool exec_softirq = false;
>
> We don't typically use "exec" especially in the context of softirqs.
> I would just change it to "softirq".
>
Ok
>
>> unsigned int cpu = smp_processor_id();
>>
>> ASSERT_NOT_IN_ATOMIC();
>> @@ -2604,7 +2604,7 @@ static void cf_check sched_slave(void)
>> return;
>> }
>>
>> - do_softirq = true;
>> + exec_softirq = true;
>> }
>>
>> if ( !prev->rendezvous_in_cnt )
>> @@ -2614,7 +2614,7 @@ static void cf_check sched_slave(void)
>> rcu_read_unlock(&sched_res_rculock);
>>
>> /* Check for failed forced context switch. */
>> - if ( do_softirq )
>> + if ( exec_softirq )
>> raise_softirq(SCHEDULE_SOFTIRQ);
>>
>> return;
>> diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
>> index 87a1e31ee9..aba51a7963 100644
>> --- a/xen/common/sched/credit2.c
>> +++ b/xen/common/sched/credit2.c
>> @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
>> list_for_each_entry ( rqd, &prv->rql, rql )
>> {
>> struct list_head *iter, *runq = &rqd->runq;
>> - int loop = 0;
>> + int it = 0;
>
> Nice catch! This is almost a bug fix.
>
>
>> /* We need the lock to scan the runqueue. */
>> spin_lock(&rqd->lock);
>> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>>
>> if ( svc )
>> {
>> - printk("\t%3d: ", loop++);
>> + printk("\t%3d: ", it++);
>> csched2_dump_unit(prv, svc);
>> }
>> }
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index 0cbfe8bd44..7cabfb0230 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -71,7 +71,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>> break;
>>
>> case XEN_SYSCTL_sched_id:
>> - op->u.sched_id.sched_id = sched_id();
>> + op->u.sched_id.sched_id = scheduler_id();
>
> I am confused about this one. There is no global variable or no other
> global function named "sched_id". Why do we need to rename sched_id to
> scheduler_id?
>
sched_id is also a common parameter name used by functions declared
where the declaration of function 'sched_id' is also visible, so it's
entirely equivalent whether to change parameter names or the function
identifier, but since there's just one usage of the function (in
'xen/common/sysctl.c') I preferred to make the minimal change in the
patch. If you prefer this done the other way around, no problem.
>
>> break;
>>
>> case XEN_SYSCTL_getdomaininfolist:
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 854f3e32c0..bfe714d2e2 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -791,7 +791,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid);
>> void sched_destroy_domain(struct domain *d);
>> long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
>> long sched_adjust_global(struct xen_sysctl_scheduler_op *);
>> -int sched_id(void);
>> +int scheduler_id(void);
>>
>> /*
>> * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
>> --
>> 2.34.1
>>
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
On Sat, 22 Jul 2023, Nicola Vetrini wrote:
> On 22/07/23 01:06, Stefano Stabellini wrote:
> > On Fri, 21 Jul 2023, Nicola Vetrini wrote:
> > > Rule 5.3 has the following headline:
> > > "An identifier declared in an inner scope shall not hide an
> > > identifier declared in an outer scope"
> > >
> > > The renaming s/sched_id/scheduler_id of the function defined in
> > > 'xen/common/sched/core.c' prevents any hiding of that function
> > > by the many instances of omonymous function parameters.
> > >
> > > Similarly, the renames
> > > - s/ops/operations
> > > - s/do_softirq/exec_softirq
> > > - s/loop/it
> > > are introduced for parameter names, to avoid any conflict
> > > with the homonymous variable or function defined in an enclosing
> > > scope.
> > >
> > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > ---
> > > xen/common/sched/core.c | 18 +++++++++---------
> > > xen/common/sched/credit2.c | 4 ++--
> > > xen/common/sysctl.c | 2 +-
> > > xen/include/xen/sched.h | 2 +-
> > > 4 files changed, 13 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > > index 022f548652..e74b1208bd 100644
> > > --- a/xen/common/sched/core.c
> > > +++ b/xen/common/sched/core.c
> > > @@ -99,13 +99,13 @@ static void sched_set_affinity(
> > > struct sched_unit *unit, const cpumask_t *hard, const cpumask_t
> > > *soft);
> > > static struct sched_resource *cf_check
> > > -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit
> > > *unit)
> > > +sched_idle_res_pick(const struct scheduler *operations, const struct
> > > sched_unit *unit)
> >
> > nit: code style, now the line is over 80 chars, could be fixed on commit
> >
>
> Ok
>
>
> > > -/* sched_id - fetch ID of current scheduler */
> > > -int sched_id(void)
> > > +/* scheduler_id - fetch ID of current scheduler */
> > > +int scheduler_id(void)
> > > {
> > > return ops.sched_id;
> > > }
> > > @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
> > > struct sched_unit *prev = vprev->sched_unit, *next;
> > > s_time_t now;
> > > spinlock_t *lock;
> > > - bool do_softirq = false;
> > > + bool exec_softirq = false;
> >
> > We don't typically use "exec" especially in the context of softirqs.
> > I would just change it to "softirq".
> >
>
> Ok
>
> >
> > > unsigned int cpu = smp_processor_id();
> > > ASSERT_NOT_IN_ATOMIC();
> > > @@ -2604,7 +2604,7 @@ static void cf_check sched_slave(void)
> > > return;
> > > }
> > > - do_softirq = true;
> > > + exec_softirq = true;
> > > }
> > > if ( !prev->rendezvous_in_cnt )
> > > @@ -2614,7 +2614,7 @@ static void cf_check sched_slave(void)
> > > rcu_read_unlock(&sched_res_rculock);
> > > /* Check for failed forced context switch. */
> > > - if ( do_softirq )
> > > + if ( exec_softirq )
> > > raise_softirq(SCHEDULE_SOFTIRQ);
> > > return;
> > > diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
> > > index 87a1e31ee9..aba51a7963 100644
> > > --- a/xen/common/sched/credit2.c
> > > +++ b/xen/common/sched/credit2.c
> > > @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
> > > list_for_each_entry ( rqd, &prv->rql, rql )
> > > {
> > > struct list_head *iter, *runq = &rqd->runq;
> > > - int loop = 0;
> > > + int it = 0;
> >
> > Nice catch! This is almost a bug fix.
> >
> >
> > > /* We need the lock to scan the runqueue. */
> > > spin_lock(&rqd->lock);
> > > @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
> > > if ( svc )
> > > {
> > > - printk("\t%3d: ", loop++);
> > > + printk("\t%3d: ", it++);
> > > csched2_dump_unit(prv, svc);
> > > }
> > > }
> > > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> > > index 0cbfe8bd44..7cabfb0230 100644
> > > --- a/xen/common/sysctl.c
> > > +++ b/xen/common/sysctl.c
> > > @@ -71,7 +71,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
> > > u_sysctl)
> > > break;
> > > case XEN_SYSCTL_sched_id:
> > > - op->u.sched_id.sched_id = sched_id();
> > > + op->u.sched_id.sched_id = scheduler_id();
> >
> > I am confused about this one. There is no global variable or no other
> > global function named "sched_id". Why do we need to rename sched_id to
> > scheduler_id?
> >
>
> sched_id is also a common parameter name used by functions declared where the
> declaration of function 'sched_id' is also visible, so it's entirely
> equivalent whether to change parameter names or the function identifier, but
> since there's just one usage of the function (in 'xen/common/sysctl.c') I
> preferred to make the minimal change in the patch. If you prefer this done the
> other way around, no problem.
I searched through the code and there aren't that many parameters or
local variables called "sched_id" maybe only 5-6. Still, thinking about
it, I think it is better to rename the function to scheduler_id() as you
did in this patch.
Cheers,
Stefano
© 2016 - 2024 Red Hat, Inc.