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

[Jürgen Groß <jgross@xxxxxxxx>]

On 18.12.19 12:54, Dario Faggioli wrote:
> 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).
I'm fine with that.

> > @@ -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?
Yes, will do that.


Juergen

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