diff mbox series

[XEN] xen/sched: mechanical renaming to address MISRA C:2012 Rule 5.3

Message ID 9945fd23b0bb88f3e0c6054a7f992cfa642d3f9f.1689953420.git.nicola.vetrini@bugseng.com (mailing list archive)
State Superseded
Headers show
Series [XEN] xen/sched: mechanical renaming to address MISRA C:2012 Rule 5.3 | expand

Commit Message

Nicola Vetrini July 21, 2023, 3:31 p.m. UTC
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(-)

Comments

Stefano Stabellini July 21, 2023, 11:06 p.m. UTC | #1
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
>
Nicola Vetrini July 22, 2023, 7:07 a.m. UTC | #2
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
>>
Jan Beulich July 24, 2023, 8:26 a.m. UTC | #3
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.

> @@ -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
Stefano Stabellini July 24, 2023, 10:22 p.m. UTC | #4
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
Nicola Vetrini July 25, 2023, 7:28 a.m. UTC | #5
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,
diff mbox series

Patch

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