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

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

[Marek Marczykowski-Górecki]

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>
---
The whole thing is under cpu_runqueue_match() already, so maybe
cpu_runqueue_siblings_match() isn't needed at all?

[resent with fixed xen-devel address...]
---
 xen/common/sched/credit2.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 0e3f89e5378e..9b8ca4d5ae24 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -996,9 +996,13 @@ 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)) )
+                  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] xen: credit2: respect credit2_runqueue=all when arranging runqueues

[Jan Beulich]

On 19.09.2022 17:09, Marek Marczykowski-Górecki wrote:
> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -996,9 +996,13 @@ 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.

This last part is questionable, partly because the command line doc is
ambiguous as to which of the two options is intended to "win". I guess
one needs to know the original intentions to resolve this.

>               */
>              if ( rqd->refcnt < max_cpus_runq && (ops->cpupool->gran != SCHED_GRAN_cpu ||
> -                  cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq)) )
> +                  cpu_runqueue_siblings_match(rqd, cpu, max_cpus_runq) ||
> +                  opt_runqueue == OPT_RUNQUEUE_ALL) )

Indentation was already broken here. As you're touching it, may I ask
that you correct that aspect?

Jan

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Tue, Sep 20, 2022 at 11:06:57AM +0200, Jan Beulich wrote:
> On 19.09.2022 17:09, Marek Marczykowski-Górecki wrote:
> > --- a/xen/common/sched/credit2.c
> > +++ b/xen/common/sched/credit2.c
> > @@ -996,9 +996,13 @@ 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.
> 
> This last part is questionable, partly because the command line doc is
> ambiguous as to which of the two options is intended to "win". I guess
> one needs to know the original intentions to resolve this.

Right, I've chosen this approach, because you can still emulate the
other by setting sufficiently large max_cpus_runq. I can add doc
clarification in v2.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1284 bytes --]

On 20.09.22 15:23, Marek Marczykowski-Górecki wrote:
> On Tue, Sep 20, 2022 at 11:06:57AM +0200, Jan Beulich wrote:
>> On 19.09.2022 17:09, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/common/sched/credit2.c
>>> +++ b/xen/common/sched/credit2.c
>>> @@ -996,9 +996,13 @@ 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.
>>
>> This last part is questionable, partly because the command line doc is
>> ambiguous as to which of the two options is intended to "win". I guess
>> one needs to know the original intentions to resolve this.
> 
> Right, I've chosen this approach, because you can still emulate the
> other by setting sufficiently large max_cpus_runq. I can add doc
> clarification in v2.
> 

I think this is the better approach, as it allows more flexibility.

Updating the doc would be mandatory, though. With that added you can
have my:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Marek Marczykowski-Górecki]

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>
---
The whole thing is under cpu_runqueue_match() already, so maybe
cpu_runqueue_siblings_match() isn't needed at all?
---
 xen/common/sched/credit2.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 0e3f89e5378e..9b8ca4d5ae24 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -996,9 +996,13 @@ 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)) )
+                  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