Re: [Xen-devel] [PATCH 7/9] xen/sched: switch scheduling to bool where appropriate

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Wed, 2019-12-18 at 08:48 +0100, Juergen Gross wrote:
> Scheduling code has several places using int or bool_t instead of
> bool.
> Switch those.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>
I'm fine with pretty much everything in this patch. Just two comments:

> diff --git a/xen/common/sched/sched_rt.c
> b/xen/common/sched/sched_rt.c
> index 264a753116..8646d77343 100644
> --- a/xen/common/sched/sched_rt.c
> +++ b/xen/common/sched/sched_rt.c
> @@ -490,10 +490,10 @@ rt_update_deadline(s_time_t now, struct rt_unit
> *svc)
>  static inline bool
>  deadline_queue_remove(struct list_head *queue, struct list_head
> *elem)
>  {
> -    int pos = 0;
> +    bool pos = false;
>  
>      if ( queue->next != elem )
> -        pos = 1;
> +        pos = true;
>  
>      list_del_init(elem);
>      return !pos;
>
IIRC, this code was "inspired" by similar functions in Credit2, where
we store in 'pos' the actual position of the entity in the runq (only
for tracing purposes, these days, but that's another story).

In here, it is indeed used only as a flag so it must be bool, and it
can also have a better name like, for instance, 'first' or 'head' (I
probably like 'first' better).

> @@ -505,14 +505,14 @@ deadline_queue_insert(struct rt_unit *
> (*qelem)(struct list_head *),
>                        struct list_head *queue)
>  {
>      struct list_head *iter;
> -    int pos = 0;
> +    bool pos = false;
>  
>      list_for_each ( iter, queue )
>      {
>          struct rt_unit * iter_svc = (*qelem)(iter);
>          if ( compare_unit_priority(svc, iter_svc) > 0 )
>              break;
> -        pos++;
> +        pos = true;
>      }
>      list_add_tail(elem, iter);
>      return !pos;
>
And this is similar, but the logic is inverted.

I think the best solution for this hunk, without subverting the code
too much, would be to also rename 'pos' into 'fist' and initialize it
to true.

Then, in the loop, we set it to false. So that it will still be true,
if we exit immediately, false if we do at least one step.

And finally we return 'first' and not its negation.

Thoughts?

Thanks and 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)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel