[Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Juergen Gross]

When dumping the run queue information add some more data regarding
current and (if known) previous vcpu for each physical cpu.

With core scheduling activated the printed data will be e.g.:

(XEN) CPUs info:
(XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
(XEN) CPU[01] current=d[IDLE]v1
(XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
(XEN) CPU[03] current=d[IDLE]v3

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: add proper locking
---
 xen/common/sched/core.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 2e43f8029f..6fbc30e678 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3234,7 +3234,7 @@ void scheduler_free(struct scheduler *sched)
 
 void schedule_dump(struct cpupool *c)
 {
-    unsigned int      i;
+    unsigned int      i, j;
     struct scheduler *sched;
     cpumask_t        *cpus;
 
@@ -3245,7 +3245,7 @@ void schedule_dump(struct cpupool *c)
     if ( c != NULL )
     {
         sched = c->sched;
-        cpus = c->cpu_valid;
+        cpus = c->res_valid;
         printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name);
         sched_dump_settings(sched);
     }
@@ -3255,11 +3255,25 @@ void schedule_dump(struct cpupool *c)
         cpus = &cpupool_free_cpus;
     }
 
-    if ( sched->dump_cpu_state != NULL )
+    printk("CPUs info:\n");
+    for_each_cpu (i, cpus)
     {
-        printk("CPUs info:\n");
-        for_each_cpu (i, cpus)
-            sched_dump_cpu_state(sched, i);
+        struct sched_resource *sr = get_sched_res(i);
+        unsigned long flags;
+        spinlock_t *lock;
+
+        lock = pcpu_schedule_lock_irqsave(i, &flags);
+
+        printk("CPU[%02d] current=%pv, curr=%pv, prev=%pv\n", i,
+               get_cpu_current(i), sr->curr ? sr->curr->vcpu_list : NULL,
+               sr->prev ? sr->prev->vcpu_list : NULL);
+        for_each_cpu (j, sr->cpus)
+            if ( i != j )
+                printk("CPU[%02d] current=%pv\n", j, get_cpu_current(j));
+
+        pcpu_schedule_unlock_irqrestore(lock, flags, i);
+
+        sched_dump_cpu_state(sched, i);
     }
 
     rcu_read_unlock(&sched_res_rculock);
-- 
2.16.4


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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Jan Beulich]

On 11.02.2020 13:27, Juergen Gross wrote:
> When dumping the run queue information add some more data regarding
> current and (if known) previous vcpu for each physical cpu.
> 
> With core scheduling activated the printed data will be e.g.:
> 
> (XEN) CPUs info:
> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
> (XEN) CPU[01] current=d[IDLE]v1
> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
> (XEN) CPU[03] current=d[IDLE]v3
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2: add proper locking

"Proper" is ambiguous in the context of dumping functions. In a
number of places we use try-lock, to avoid the dumping hanging
on something else monopolizing the lock. I'd like to suggest to
do so here, too.

Jan

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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Jürgen Groß]

On 11.02.20 14:01, Jan Beulich wrote:
> On 11.02.2020 13:27, Juergen Gross wrote:
>> When dumping the run queue information add some more data regarding
>> current and (if known) previous vcpu for each physical cpu.
>>
>> With core scheduling activated the printed data will be e.g.:
>>
>> (XEN) CPUs info:
>> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
>> (XEN) CPU[01] current=d[IDLE]v1
>> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
>> (XEN) CPU[03] current=d[IDLE]v3
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2: add proper locking
> 
> "Proper" is ambiguous in the context of dumping functions. In a
> number of places we use try-lock, to avoid the dumping hanging
> on something else monopolizing the lock. I'd like to suggest to
> do so here, too.

All the scheduler related dumping functions are using the "real" locks.
So using trylock in this single case wouldn't help at all. Additionally
using trylock only would make a crash during dumping the data more
probable, so I'm not sure we want that.

Instead of unconditionally using trylock in dumping functions I could
imagine to have a "dumplock" using proper locking by default which can
be toggled to trylock in case it is needed (or maybe automatically by
adding a timeout to the dumplock variant).


Juergen

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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Jan Beulich]

On 11.02.2020 14:10, Jürgen Groß wrote:
> On 11.02.20 14:01, Jan Beulich wrote:
>> On 11.02.2020 13:27, Juergen Gross wrote:
>>> When dumping the run queue information add some more data regarding
>>> current and (if known) previous vcpu for each physical cpu.
>>>
>>> With core scheduling activated the printed data will be e.g.:
>>>
>>> (XEN) CPUs info:
>>> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
>>> (XEN) CPU[01] current=d[IDLE]v1
>>> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
>>> (XEN) CPU[03] current=d[IDLE]v3
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2: add proper locking
>>
>> "Proper" is ambiguous in the context of dumping functions. In a
>> number of places we use try-lock, to avoid the dumping hanging
>> on something else monopolizing the lock. I'd like to suggest to
>> do so here, too.
> 
> All the scheduler related dumping functions are using the "real" locks.
> So using trylock in this single case wouldn't help at all. Additionally
> using trylock only would make a crash during dumping the data more
> probable, so I'm not sure we want that.

Why would it make a crash more likely? If you can't get the lock,
you'd simply skip dumping.

Jan

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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Jürgen Groß]

On 11.02.20 17:46, Jan Beulich wrote:
> On 11.02.2020 14:10, Jürgen Groß wrote:
>> On 11.02.20 14:01, Jan Beulich wrote:
>>> On 11.02.2020 13:27, Juergen Gross wrote:
>>>> When dumping the run queue information add some more data regarding
>>>> current and (if known) previous vcpu for each physical cpu.
>>>>
>>>> With core scheduling activated the printed data will be e.g.:
>>>>
>>>> (XEN) CPUs info:
>>>> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
>>>> (XEN) CPU[01] current=d[IDLE]v1
>>>> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
>>>> (XEN) CPU[03] current=d[IDLE]v3
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V2: add proper locking
>>>
>>> "Proper" is ambiguous in the context of dumping functions. In a
>>> number of places we use try-lock, to avoid the dumping hanging
>>> on something else monopolizing the lock. I'd like to suggest to
>>> do so here, too.
>>
>> All the scheduler related dumping functions are using the "real" locks.
>> So using trylock in this single case wouldn't help at all. Additionally
>> using trylock only would make a crash during dumping the data more
>> probable, so I'm not sure we want that.
> 
> Why would it make a crash more likely? If you can't get the lock,
> you'd simply skip dumping.

Ah, okay, then I misunderstood your intention.

I still think that this should be done not only in one place, but in a
more general fashion. I'd rather give up only after some time trying
(1 millisecond per default?) and apply the same scheme to all dumping
functions.

I can have a try for such a series if you agree on taking a more general
approach.


Juergen

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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Jan Beulich]

On 11.02.2020 17:54, Jürgen Groß wrote:
> On 11.02.20 17:46, Jan Beulich wrote:
>> On 11.02.2020 14:10, Jürgen Groß wrote:
>>> On 11.02.20 14:01, Jan Beulich wrote:
>>>> On 11.02.2020 13:27, Juergen Gross wrote:
>>>>> When dumping the run queue information add some more data regarding
>>>>> current and (if known) previous vcpu for each physical cpu.
>>>>>
>>>>> With core scheduling activated the printed data will be e.g.:
>>>>>
>>>>> (XEN) CPUs info:
>>>>> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
>>>>> (XEN) CPU[01] current=d[IDLE]v1
>>>>> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
>>>>> (XEN) CPU[03] current=d[IDLE]v3
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> V2: add proper locking
>>>>
>>>> "Proper" is ambiguous in the context of dumping functions. In a
>>>> number of places we use try-lock, to avoid the dumping hanging
>>>> on something else monopolizing the lock. I'd like to suggest to
>>>> do so here, too.
>>>
>>> All the scheduler related dumping functions are using the "real" locks.
>>> So using trylock in this single case wouldn't help at all. Additionally
>>> using trylock only would make a crash during dumping the data more
>>> probable, so I'm not sure we want that.
>>
>> Why would it make a crash more likely? If you can't get the lock,
>> you'd simply skip dumping.
> 
> Ah, okay, then I misunderstood your intention.
> 
> I still think that this should be done not only in one place, but in a
> more general fashion. I'd rather give up only after some time trying
> (1 millisecond per default?) and apply the same scheme to all dumping
> functions.
> 
> I can have a try for such a series if you agree on taking a more general
> approach.

Getting behavior consistent across key handlers would of course
be very nice.

Jan

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

Re: [Xen-devel] [PATCH v2] xen/sched: add some diagnostic info in the run queue keyhandler

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1352 bytes --]

On Tue, 2020-02-11 at 13:27 +0100, Juergen Gross wrote:
> When dumping the run queue information add some more data regarding
> current and (if known) previous vcpu for each physical cpu.
> 
> With core scheduling activated the printed data will be e.g.:
> 
> (XEN) CPUs info:
> (XEN) CPU[00] current=d[IDLE]v0, curr=d[IDLE]v0, prev=NULL
> (XEN) CPU[01] current=d[IDLE]v1
> (XEN) CPU[02] current=d[IDLE]v2, curr=d[IDLE]v2, prev=NULL
> (XEN) CPU[03] current=d[IDLE]v3
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
I'm fine with this patch, i.e., I think the additional logging it is
adding is useful and the locking is correct and in line with the
current (scheduling) keyhandler locking practices.

And thanks for making the requested adjustments to the changelog. :-)

So, I think it should go in:

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

If/when, as a result of the discussion going on in the thread about
keyhandler locking, we decide to change all the locks for all the
keyhandlers, then we'll change them here as well.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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