Re: [PATCH v2 4/7] xen: credit2: limit the max number of CPUs in a runqueue

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Fri, 2020-05-29 at 17:23 +0200, Jürgen Groß wrote:
> On 28.05.20 23:29, Dario Faggioli wrote:
> > In Credit2 CPUs (can) share runqueues, depending on the topology.
> > For
> > instance, with per-socket runqueues (the default) all the CPUs that
> > are
> > part of the same socket share a runqueue.
> > 
> > On platform with a huge number of CPUs per socket, that could be a
> > problem. An example is AMD EPYC2 servers, where we can have up to
> > 128
> > CPUs in a socket.
> > 
> > It is of course possible to define other, still topology-based,
> > runqueue
> > arrangements (e.g., per-LLC, per-DIE, etc). But that may still
> > result in
> > runqueues with too many CPUs on other/future platforms. For
> > instance, a
> > system with 96 CPUs and 2 NUMA nodes will end up having 48 CPUs per
> > runqueue. Not as bad, but still a lot!
> > 
> > Therefore, let's set a limit to the max number of CPUs that can
> > share a
> > Credit2 runqueue. The actual value is configurable (at boot time),
> > the
> > default being 16. If, for instance,  there are more than 16 CPUs in
> > a
> > socket, they'll be split among two (or more) runqueues.
> > 
> > Note: with core scheduling enabled, this parameter sets the max
> > number
> > of *scheduling resources* that can share a runqueue. Therefore,
> > with
> > granularity set to core (and assumint 2 threads per core), we will
> > have
> > at most 16 cores per runqueue, which corresponds to 32 threads. But
> > that
> > is fine, considering how core scheduling works.
> > 
> > Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> 
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
> 
Thanks!

> with one additional question below.
> 
Sure...

> > +            if ( cpumask_intersects(&rqd->active,
> > per_cpu(cpu_sibling_mask, cpu)) )
> > +            {
> > +                if ( cpumask_weight(&rqd->active) >=
> > opt_max_cpus_runqueue )
> > +                {
> > +                        printk("WARNING: %s: more than
> > opt_max_cpus_runqueue "
> > +                               "in a runqueue (%u vs %u), due to
> > topology constraints.\n"
> > +                               "Consider raising it!\n",
> > +                               __func__, opt_max_cpus_runqueue,
> 
> Is printing the function name really adding any important
> information?
> 
I personally don't think it does. I am mostly following suite from both
this file and other places, even for this kind of warnings, striving
for consistency.

I'd surely be fine removing it from all the warnings about the boot
time parameter, here in credit2 and in scheduling in general. And if
now it's not the time for doing that, I'm happy to get rid of it from
here, and do the rest later. Or to leave it everywhere for now but
remove it from everywhere later.

And I don't have a too strong opinion myself, so, whatever we think is
best, I can do it.

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