Re: [Xen-devel] [PATCH] Xen sched: Fix multiple runqueues in credit2

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On gio, 2014-02-06 at 11:17 +0000, Jan Beulich wrote:
> >>> On 06.02.14 at 09:58, Justin Weaver <jtweaver@xxxxxxxxxx> wrote:
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -85,8 +85,7 @@
> >   * to a small value, and a fixed credit is added to everyone.
> >   *
> >   * The plan is for all cores that share an L2 will share the same
> > - * runqueue.  At the moment, there is one global runqueue for all
> > - * cores.
> > + * runqueue. 
> 
> If this is the intention, then ...
>
> [...]
>
> ... this is too simplistic: Whether the L2 is shared by all cores on a
> socket should be determined, not assumed.
> 
True. However, what we do right now is trying to build one runqueu per
socket, by means of cpu_to_socket(), and failing badly, ending up with
only one *system wide* runqueue. Personally, I think that fixing this,
i.e., keeping using cpu_to_socket(), but in a correct way, and actually
building '1 runqueue per socket' would be a reasonable step in the
original author's intentions. Of course, in this case, I concur that the
comment above needs changing too.

Thoughts?

> Apart from that keeping the CPU0 special case at the top is pointless
> with the cpu0_socket special casing.
> 
Indeed. If going this route, Justin, I think you can reorganize the
whole `if (cpu == 0)' (not only the else), and get to a more correct and
readable solution.

> As to coding style: Please fix your comments and get the indentation
> of the if/else sequence above right (i.e. either use "else if" with no
> added indentation, or enclose the inner if/else in figure braces (I'd
> personally prefer the former).
> 
Yep, agreed. To be fair, about comments, sched_credit2.c has quite a
mixture of commenting style in it, and it's really an hard call to
decide which one should be used. Anyway, Justin, if you reorganize the
whole `if () else' block, you are probably better off with a big comment
describing the whole thing, before the block itself, for which you can
use the following style:

/*
 * Long comment...
 */

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel