[v2] xen: credit2: respect credit2_runqueue=all when arranging runqueues

[PATCH v2] xen: credit2: respect credit2_runqueue=all when arranging runqueues

Posted by Marek Marczykowski-Górecki 1 year, 7 months ago

Documentation for credit2_runqueue=all says it should create one queue
for all pCPUs on the host. But since introduction
sched_credit2_max_cpus_runqueue, it actually created separate runqueue
per socket, even if the CPUs count is below
sched_credit2_max_cpus_runqueue.

Adjust the condition to skip syblink check in case of
credit2_runqueue=all.

Fixes: 8e2aa76dc167 ("xen: credit2: limit the max number of CPUs in a runqueue")
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Changes in v2:
 - fix indentation
 - adjust doc

The whole thing is under cpu_runqueue_match() already, so maybe
cpu_runqueue_siblings_match() isn't needed at all?
---
 docs/misc/xen-command-line.pandoc | 5 +++++
 xen/common/sched/credit2.c        | 9 +++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 74b519f0c5bd..057cdb903042 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -724,6 +724,11 @@ Available alternatives, with their meaning, are:
 * `all`: just one runqueue shared by all the logical pCPUs of
          the host
 
+Regardless of the above choice, Xen attempts to respect
+`sched_credit2_max_cpus_runqueue` limit, which may mean more than one runqueue
+for the `all` value. If that isn't intended, raise
+the `sched_credit2_max_cpus_runqueue` value.
+
 ### dbgp
 > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
 > `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=<bool>|hwdom]`
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 0e3f89e5378e..afff23b56238 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -996,9 +996,14 @@ cpu_add_to_runqueue(const struct scheduler *ops, unsigned int cpu)
              *
              * Otherwise, let's try to make sure that siblings stay in the
              * same runqueue, pretty much under any cinrcumnstances.
+             *
+             * Furthermore, try to respect credit2_runqueue=all, as long as
+             * max_cpus_runq isn't violated.
              */
-            if ( rqd->refcnt < max_cpus_runq && (ops->cpupool->gran != SCHED_GRAN_cpu ||
-                  cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq)) )
+            if ( rqd->refcnt < max_cpus_runq &&
+                    (ops->cpupool->gran != SCHED_GRAN_cpu ||
+                     cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq) ||
+                     opt_runqueue == OPT_RUNQUEUE_ALL) )
             {
                 /*
                  * This runqueue is ok, but as we said, we also want an even
-- 
2.37.3

Re: [PATCH v2] xen: credit2: respect credit2_runqueue=all when arranging runqueues

Posted by Jan Beulich 1 year, 2 months ago

On 03.10.2022 18:21, Marek Marczykowski-Górecki wrote:
> Documentation for credit2_runqueue=all says it should create one queue
> for all pCPUs on the host. But since introduction
> sched_credit2_max_cpus_runqueue, it actually created separate runqueue
> per socket, even if the CPUs count is below
> sched_credit2_max_cpus_runqueue.
> 
> Adjust the condition to skip syblink check in case of
> credit2_runqueue=all.
> 
> Fixes: 8e2aa76dc167 ("xen: credit2: limit the max number of CPUs in a runqueue")
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Juergen Gross <jgross@suse.com>

I've now committed this without maintainer ack.

> Changes in v2:
>  - fix indentation

I didn't go check v1, but ...

> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -996,9 +996,14 @@ cpu_add_to_runqueue(const struct scheduler *ops, unsigned int cpu)
>               *
>               * Otherwise, let's try to make sure that siblings stay in the
>               * same runqueue, pretty much under any cinrcumnstances.
> +             *
> +             * Furthermore, try to respect credit2_runqueue=all, as long as
> +             * max_cpus_runq isn't violated.
>               */
> -            if ( rqd->refcnt < max_cpus_runq && (ops->cpupool->gran != SCHED_GRAN_cpu ||
> -                  cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq)) )
> +            if ( rqd->refcnt < max_cpus_runq &&
> +                    (ops->cpupool->gran != SCHED_GRAN_cpu ||
> +                     cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq) ||
> +                     opt_runqueue == OPT_RUNQUEUE_ALL) )

... this still looked like too deep indentation to me. I've taken the
liberty to adjust this while committing.

Jan

Ping: [PATCH v2] xen: credit2: respect credit2_runqueue=all when arranging runqueues

Posted by Jan Beulich 1 year, 4 months ago

On 03.10.2022 18:21, Marek Marczykowski-Górecki wrote:
> Documentation for credit2_runqueue=all says it should create one queue
> for all pCPUs on the host. But since introduction
> sched_credit2_max_cpus_runqueue, it actually created separate runqueue
> per socket, even if the CPUs count is below
> sched_credit2_max_cpus_runqueue.
> 
> Adjust the condition to skip syblink check in case of
> credit2_runqueue=all.
> 
> Fixes: 8e2aa76dc167 ("xen: credit2: limit the max number of CPUs in a runqueue")
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Juergen Gross <jgross@suse.com>
> ---
> Changes in v2:
>  - fix indentation
>  - adjust doc
> 
> The whole thing is under cpu_runqueue_match() already, so maybe
> cpu_runqueue_siblings_match() isn't needed at all?
> ---
>  docs/misc/xen-command-line.pandoc | 5 +++++
>  xen/common/sched/credit2.c        | 9 +++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)

George, Dario - any chance of an ack (or reasons not to)?

Jan

Ping²: [PATCH v2] xen: credit2: respect credit2_runqueue=all when arranging runqueues

Posted by Jan Beulich 1 year, 2 months ago

On 07.12.2022 12:41, Jan Beulich wrote:
> On 03.10.2022 18:21, Marek Marczykowski-Górecki wrote:
>> Documentation for credit2_runqueue=all says it should create one queue
>> for all pCPUs on the host. But since introduction
>> sched_credit2_max_cpus_runqueue, it actually created separate runqueue
>> per socket, even if the CPUs count is below
>> sched_credit2_max_cpus_runqueue.
>>
>> Adjust the condition to skip syblink check in case of
>> credit2_runqueue=all.
>>
>> Fixes: 8e2aa76dc167 ("xen: credit2: limit the max number of CPUs in a runqueue")
>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
>> ---
>> Changes in v2:
>>  - fix indentation
>>  - adjust doc
>>
>> The whole thing is under cpu_runqueue_match() already, so maybe
>> cpu_runqueue_siblings_match() isn't needed at all?
>> ---
>>  docs/misc/xen-command-line.pandoc | 5 +++++
>>  xen/common/sched/credit2.c        | 9 +++++++--
>>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> George, Dario - any chance of an ack (or reasons not to)?

Another two months later I'm inclined to commit this with just Jürgen's R-b
(assuming it still applies cleanly). I'll give it a day or two more ...

Jan