[PATCH 0/2] xen: sched/Credit2: small and trivial improvements

[PATCH 0/2] xen: sched/Credit2: small and trivial improvements

[Dario Faggioli]

Namely:
1. get rid of cpumask_weight() in hot paths, in Credit2;
2. checking whether prev==next in context_switch() is pointless.

Regards,
Dario
---
Dario Faggioli (2):
      xen: credit2: avoid using cpumask_weight() in hot-paths
      xen: sched: we never get into context_switch() with prev==next

 xen/arch/arm/domain.c      |    3 +--
 xen/arch/x86/domain.c      |   22 ++++++++--------------
 xen/common/sched_credit2.c |   21 ++++++++++++++++-----
 3 files changed, 25 insertions(+), 21 deletions(-)

--
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)

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

[Xen-devel] [PATCH 0/2] xen: sched/Credit2: small and trivial improvements

[Dario Faggioli]

Namely:
1. get rid of cpumask_weight() in hot paths, in Credit2;
2. checking whether prev==next in context_switch() is pointless.

Regards,
Dario
---
Dario Faggioli (2):
      xen: credit2: avoid using cpumask_weight() in hot-paths
      xen: sched: we never get into context_switch() with prev==next

 xen/arch/arm/domain.c      |    3 +--
 xen/arch/x86/domain.c      |   22 ++++++++--------------
 xen/common/sched_credit2.c |   21 ++++++++++++++++-----
 3 files changed, 25 insertions(+), 21 deletions(-)

--
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)

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

[PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Dario Faggioli]

cpumask_weight() is known to be expensive. In Credit2, we use it in
load-balancing, but only for knowing how many CPUs are active in a
runqueue.

Keeping such count in an integer field of the per-runqueue data
structure we have, completely avoids the need for cpumask_weight().

While there, remove as much other uses of it as we can, even if not in
hot-paths.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_credit2.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6958b265fc..7034325243 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -466,6 +466,7 @@ struct csched2_runqueue_data {
     spinlock_t lock;           /* Lock for this runqueue                     */
 
     struct list_head runq;     /* Ordered list of runnable vms               */
+    int nr_cpus;               /* How many CPUs are sharing this runqueue    */
     int id;                    /* ID of this runqueue (-1 if invalid)        */
 
     int load;                  /* Instantaneous load (num of non-idle vcpus) */
@@ -2613,8 +2614,8 @@ retry:
         if ( st.orqd->b_avgload > load_max )
             load_max = st.orqd->b_avgload;
 
-        cpus_max = cpumask_weight(&st.lrqd->active);
-        i = cpumask_weight(&st.orqd->active);
+        cpus_max = st.lrqd->nr_cpus;
+        i = st.orqd->nr_cpus;
         if ( i > cpus_max )
             cpus_max = i;
 
@@ -3697,7 +3698,7 @@ csched2_dump(const struct scheduler *ops)
                "\tinstload           = %d\n"
                "\taveload            = %"PRI_stime" (~%"PRI_stime"%%)\n",
                i,
-               cpumask_weight(&prv->rqd[i].active),
+               prv->rqd[i].nr_cpus,
                nr_cpu_ids, cpumask_bits(&prv->rqd[i].active),
                prv->rqd[i].max_weight,
                prv->rqd[i].pick_bias,
@@ -3818,6 +3819,9 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
     __cpumask_set_cpu(cpu, &prv->initialized);
     __cpumask_set_cpu(cpu, &rqd->smt_idle);
 
+    rqd->nr_cpus++;
+    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
+
     /* On the boot cpu we are called before cpu_sibling_mask has been set up. */
     if ( cpu == 0 && system_state < SYS_STATE_active )
         __cpumask_set_cpu(cpu, &csched2_pcpu(cpu)->sibling_mask);
@@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
                 __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
             }
 
-    if ( cpumask_weight(&rqd->active) == 1 )
+    if ( rqd->nr_cpus == 1 )
+    {
+	ASSERT(cpumask_weight(&rqd->active) == 1);
         rqd->pick_bias = cpu;
+    }
 
     return spc->runq_id;
 }
@@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
     __cpumask_clear_cpu(cpu, &rqd->smt_idle);
     __cpumask_clear_cpu(cpu, &rqd->active);
 
-    if ( cpumask_empty(&rqd->active) )
+    rqd->nr_cpus--;
+    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
+
+    if ( rqd->nr_cpus == 0 )
     {
+	ASSERT(cpumask_empty(&rqd->active));
         printk(XENLOG_INFO " No cpus left on runqueue, disabling\n");
         deactivate_runqueue(prv, spc->runq_id);
     }


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

[Xen-devel] [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Dario Faggioli]

cpumask_weight() is known to be expensive. In Credit2, we use it in
load-balancing, but only for knowing how many CPUs are active in a
runqueue.

Keeping such count in an integer field of the per-runqueue data
structure we have, completely avoids the need for cpumask_weight().

While there, remove as much other uses of it as we can, even if not in
hot-paths.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_credit2.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6958b265fc..7034325243 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -466,6 +466,7 @@ struct csched2_runqueue_data {
     spinlock_t lock;           /* Lock for this runqueue                     */
 
     struct list_head runq;     /* Ordered list of runnable vms               */
+    int nr_cpus;               /* How many CPUs are sharing this runqueue    */
     int id;                    /* ID of this runqueue (-1 if invalid)        */
 
     int load;                  /* Instantaneous load (num of non-idle vcpus) */
@@ -2613,8 +2614,8 @@ retry:
         if ( st.orqd->b_avgload > load_max )
             load_max = st.orqd->b_avgload;
 
-        cpus_max = cpumask_weight(&st.lrqd->active);
-        i = cpumask_weight(&st.orqd->active);
+        cpus_max = st.lrqd->nr_cpus;
+        i = st.orqd->nr_cpus;
         if ( i > cpus_max )
             cpus_max = i;
 
@@ -3697,7 +3698,7 @@ csched2_dump(const struct scheduler *ops)
                "\tinstload           = %d\n"
                "\taveload            = %"PRI_stime" (~%"PRI_stime"%%)\n",
                i,
-               cpumask_weight(&prv->rqd[i].active),
+               prv->rqd[i].nr_cpus,
                nr_cpu_ids, cpumask_bits(&prv->rqd[i].active),
                prv->rqd[i].max_weight,
                prv->rqd[i].pick_bias,
@@ -3818,6 +3819,9 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
     __cpumask_set_cpu(cpu, &prv->initialized);
     __cpumask_set_cpu(cpu, &rqd->smt_idle);
 
+    rqd->nr_cpus++;
+    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
+
     /* On the boot cpu we are called before cpu_sibling_mask has been set up. */
     if ( cpu == 0 && system_state < SYS_STATE_active )
         __cpumask_set_cpu(cpu, &csched2_pcpu(cpu)->sibling_mask);
@@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
                 __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
             }
 
-    if ( cpumask_weight(&rqd->active) == 1 )
+    if ( rqd->nr_cpus == 1 )
+    {
+	ASSERT(cpumask_weight(&rqd->active) == 1);
         rqd->pick_bias = cpu;
+    }
 
     return spc->runq_id;
 }
@@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
     __cpumask_clear_cpu(cpu, &rqd->smt_idle);
     __cpumask_clear_cpu(cpu, &rqd->active);
 
-    if ( cpumask_empty(&rqd->active) )
+    rqd->nr_cpus--;
+    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
+
+    if ( rqd->nr_cpus == 0 )
     {
+	ASSERT(cpumask_empty(&rqd->active));
         printk(XENLOG_INFO " No cpus left on runqueue, disabling\n");
         deactivate_runqueue(prv, spc->runq_id);
     }


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

[PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

In schedule(), if we pick, as the next vcpu to run (next) the same one
that is running already (prev), we never get to call context_switch().

We can, therefore, get rid of all the `if`-s testing prev and next being
different, trading them with an ASSERT() (on ARM, the ASSERT() was even
already there!)

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
Suggested-by: Juergen Gross <jgross@suse.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/domain.c |    3 +--
 xen/arch/x86/domain.c |   22 ++++++++--------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6dc633ed50..915ae0b4c6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     ASSERT(prev != next);
     ASSERT(!vcpu_cpu_dirty(next));
 
-    if ( prev != next )
-        update_runstate_area(prev);
+    update_runstate_area(prev);
 
     local_irq_disable();
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9eaa978ce5..d2d9f2fc3c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     const struct domain *prevd = prev->domain, *nextd = next->domain;
     unsigned int dirty_cpu = next->dirty_cpu;
 
+    ASSERT(prev != next);
     ASSERT(local_irq_is_enabled());
 
     get_cpu_info()->use_pv_cr3 = false;
@@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
         flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
     }
 
-    if ( prev != next )
-    {
-        _update_runstate_area(prev);
-        vpmu_switch_from(prev);
-        np2m_schedule(NP2M_SCHEDLE_OUT);
-    }
+    _update_runstate_area(prev);
+    vpmu_switch_from(prev);
+    np2m_schedule(NP2M_SCHEDLE_OUT);
 
     if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
         pt_save_timer(prev);
@@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
     context_saved(prev);
 
-    if ( prev != next )
-    {
-        _update_runstate_area(next);
-
-        /* Must be done with interrupts enabled */
-        vpmu_switch_to(next);
-        np2m_schedule(NP2M_SCHEDLE_IN);
-    }
+    _update_runstate_area(next);
+    /* Must be done with interrupts enabled */
+    vpmu_switch_to(next);
+    np2m_schedule(NP2M_SCHEDLE_IN);
 
     /* Ensure that the vcpu has an up-to-date time base. */
     update_vcpu_system_time(next);


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

[Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

In schedule(), if we pick, as the next vcpu to run (next) the same one
that is running already (prev), we never get to call context_switch().

We can, therefore, get rid of all the `if`-s testing prev and next being
different, trading them with an ASSERT() (on ARM, the ASSERT() was even
already there!)

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
Suggested-by: Juergen Gross <jgross@suse.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/domain.c |    3 +--
 xen/arch/x86/domain.c |   22 ++++++++--------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6dc633ed50..915ae0b4c6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     ASSERT(prev != next);
     ASSERT(!vcpu_cpu_dirty(next));
 
-    if ( prev != next )
-        update_runstate_area(prev);
+    update_runstate_area(prev);
 
     local_irq_disable();
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9eaa978ce5..d2d9f2fc3c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     const struct domain *prevd = prev->domain, *nextd = next->domain;
     unsigned int dirty_cpu = next->dirty_cpu;
 
+    ASSERT(prev != next);
     ASSERT(local_irq_is_enabled());
 
     get_cpu_info()->use_pv_cr3 = false;
@@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
         flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
     }
 
-    if ( prev != next )
-    {
-        _update_runstate_area(prev);
-        vpmu_switch_from(prev);
-        np2m_schedule(NP2M_SCHEDLE_OUT);
-    }
+    _update_runstate_area(prev);
+    vpmu_switch_from(prev);
+    np2m_schedule(NP2M_SCHEDLE_OUT);
 
     if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
         pt_save_timer(prev);
@@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
     context_saved(prev);
 
-    if ( prev != next )
-    {
-        _update_runstate_area(next);
-
-        /* Must be done with interrupts enabled */
-        vpmu_switch_to(next);
-        np2m_schedule(NP2M_SCHEDLE_IN);
-    }
+    _update_runstate_area(next);
+    /* Must be done with interrupts enabled */
+    vpmu_switch_to(next);
+    np2m_schedule(NP2M_SCHEDLE_IN);
 
     /* Ensure that the vcpu has an up-to-date time base. */
     update_vcpu_system_time(next);


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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Julien Grall]

Hi Dario,

On 4/20/19 4:24 PM, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

For Arm:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   xen/arch/arm/domain.c |    3 +--
>   xen/arch/x86/domain.c |   22 ++++++++--------------
>   2 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6dc633ed50..915ae0b4c6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       ASSERT(prev != next);
>       ASSERT(!vcpu_cpu_dirty(next));
>   
> -    if ( prev != next )
> -        update_runstate_area(prev);
> +    update_runstate_area(prev);
>   
>       local_irq_disable();
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9eaa978ce5..d2d9f2fc3c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       const struct domain *prevd = prev->domain, *nextd = next->domain;
>       unsigned int dirty_cpu = next->dirty_cpu;
>   
> +    ASSERT(prev != next);
>       ASSERT(local_irq_is_enabled());
>   
>       get_cpu_info()->use_pv_cr3 = false;
> @@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>           flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
>       }
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(prev);
> -        vpmu_switch_from(prev);
> -        np2m_schedule(NP2M_SCHEDLE_OUT);
> -    }
> +    _update_runstate_area(prev);
> +    vpmu_switch_from(prev);
> +    np2m_schedule(NP2M_SCHEDLE_OUT);
>   
>       if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
>           pt_save_timer(prev);
> @@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>   
>       context_saved(prev);
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(next);
> -
> -        /* Must be done with interrupts enabled */
> -        vpmu_switch_to(next);
> -        np2m_schedule(NP2M_SCHEDLE_IN);
> -    }
> +    _update_runstate_area(next);
> +    /* Must be done with interrupts enabled */
> +    vpmu_switch_to(next);
> +    np2m_schedule(NP2M_SCHEDLE_IN);
>   
>       /* Ensure that the vcpu has an up-to-date time base. */
>       update_vcpu_system_time(next);
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Julien Grall]

Hi Dario,

On 4/20/19 4:24 PM, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

For Arm:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   xen/arch/arm/domain.c |    3 +--
>   xen/arch/x86/domain.c |   22 ++++++++--------------
>   2 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6dc633ed50..915ae0b4c6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       ASSERT(prev != next);
>       ASSERT(!vcpu_cpu_dirty(next));
>   
> -    if ( prev != next )
> -        update_runstate_area(prev);
> +    update_runstate_area(prev);
>   
>       local_irq_disable();
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9eaa978ce5..d2d9f2fc3c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       const struct domain *prevd = prev->domain, *nextd = next->domain;
>       unsigned int dirty_cpu = next->dirty_cpu;
>   
> +    ASSERT(prev != next);
>       ASSERT(local_irq_is_enabled());
>   
>       get_cpu_info()->use_pv_cr3 = false;
> @@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>           flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
>       }
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(prev);
> -        vpmu_switch_from(prev);
> -        np2m_schedule(NP2M_SCHEDLE_OUT);
> -    }
> +    _update_runstate_area(prev);
> +    vpmu_switch_from(prev);
> +    np2m_schedule(NP2M_SCHEDLE_OUT);
>   
>       if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
>           pt_save_timer(prev);
> @@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>   
>       context_saved(prev);
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(next);
> -
> -        /* Must be done with interrupts enabled */
> -        vpmu_switch_to(next);
> -        np2m_schedule(NP2M_SCHEDLE_IN);
> -    }
> +    _update_runstate_area(next);
> +    /* Must be done with interrupts enabled */
> +    vpmu_switch_to(next);
> +    np2m_schedule(NP2M_SCHEDLE_IN);
>   
>       /* Ensure that the vcpu has an up-to-date time base. */
>       update_vcpu_system_time(next);
> 

-- 
Julien Grall

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Dario,

On 20.04.19 18:24, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().

And what about `if ( prev != current )` in arm/domain.c:schedule_tail() ?

> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

For all below:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   xen/arch/arm/domain.c |    3 +--
>   xen/arch/x86/domain.c |   22 ++++++++--------------
>   2 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6dc633ed50..915ae0b4c6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       ASSERT(prev != next);
>       ASSERT(!vcpu_cpu_dirty(next));
>   
> -    if ( prev != next )
> -        update_runstate_area(prev);
> +    update_runstate_area(prev);
>   
>       local_irq_disable();
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9eaa978ce5..d2d9f2fc3c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       const struct domain *prevd = prev->domain, *nextd = next->domain;
>       unsigned int dirty_cpu = next->dirty_cpu;
>   
> +    ASSERT(prev != next);
>       ASSERT(local_irq_is_enabled());
>   
>       get_cpu_info()->use_pv_cr3 = false;
> @@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>           flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
>       }
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(prev);
> -        vpmu_switch_from(prev);
> -        np2m_schedule(NP2M_SCHEDLE_OUT);
> -    }
> +    _update_runstate_area(prev);
> +    vpmu_switch_from(prev);
> +    np2m_schedule(NP2M_SCHEDLE_OUT);
>   
>       if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
>           pt_save_timer(prev);
> @@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>   
>       context_saved(prev);
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(next);
> -
> -        /* Must be done with interrupts enabled */
> -        vpmu_switch_to(next);
> -        np2m_schedule(NP2M_SCHEDLE_IN);
> -    }
> +    _update_runstate_area(next);
> +    /* Must be done with interrupts enabled */
> +    vpmu_switch_to(next);
> +    np2m_schedule(NP2M_SCHEDLE_IN);
>   
>       /* Ensure that the vcpu has an up-to-date time base. */
>       update_vcpu_system_time(next);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Dario,

On 20.04.19 18:24, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().

And what about `if ( prev != current )` in arm/domain.c:schedule_tail() ?

> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

For all below:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   xen/arch/arm/domain.c |    3 +--
>   xen/arch/x86/domain.c |   22 ++++++++--------------
>   2 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6dc633ed50..915ae0b4c6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -343,8 +343,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       ASSERT(prev != next);
>       ASSERT(!vcpu_cpu_dirty(next));
>   
> -    if ( prev != next )
> -        update_runstate_area(prev);
> +    update_runstate_area(prev);
>   
>       local_irq_disable();
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9eaa978ce5..d2d9f2fc3c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1721,6 +1721,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>       const struct domain *prevd = prev->domain, *nextd = next->domain;
>       unsigned int dirty_cpu = next->dirty_cpu;
>   
> +    ASSERT(prev != next);
>       ASSERT(local_irq_is_enabled());
>   
>       get_cpu_info()->use_pv_cr3 = false;
> @@ -1732,12 +1733,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>           flush_mask(cpumask_of(dirty_cpu), FLUSH_VCPU_STATE);
>       }
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(prev);
> -        vpmu_switch_from(prev);
> -        np2m_schedule(NP2M_SCHEDLE_OUT);
> -    }
> +    _update_runstate_area(prev);
> +    vpmu_switch_from(prev);
> +    np2m_schedule(NP2M_SCHEDLE_OUT);
>   
>       if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
>           pt_save_timer(prev);
> @@ -1794,14 +1792,10 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>   
>       context_saved(prev);
>   
> -    if ( prev != next )
> -    {
> -        _update_runstate_area(next);
> -
> -        /* Must be done with interrupts enabled */
> -        vpmu_switch_to(next);
> -        np2m_schedule(NP2M_SCHEDLE_IN);
> -    }
> +    _update_runstate_area(next);
> +    /* Must be done with interrupts enabled */
> +    vpmu_switch_to(next);
> +    np2m_schedule(NP2M_SCHEDLE_IN);
>   
>       /* Ensure that the vcpu has an up-to-date time base. */
>       update_vcpu_system_time(next);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Andrew Cooper]

On 20/04/2019 16:24, Dario Faggioli wrote:
> cpumask_weight() is known to be expensive. In Credit2, we use it in
> load-balancing, but only for knowing how many CPUs are active in a
> runqueue.

With a few small changes it can be improved substantially (by using
POPCNT when available), but it is still fundamentally an O(n) operation.

Definitely +1 to algorithm changes which avoid its use entirely.

A few cosmetic observations.

> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 6958b265fc..7034325243 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
>      spinlock_t lock;           /* Lock for this runqueue                     */
>  
>      struct list_head runq;     /* Ordered list of runnable vms               */
> +    int nr_cpus;               /* How many CPUs are sharing this runqueue    */

Unsigned int.

> @@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>                  __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
>              }
>  
> -    if ( cpumask_weight(&rqd->active) == 1 )
> +    if ( rqd->nr_cpus == 1 )
> +    {
> +	ASSERT(cpumask_weight(&rqd->active) == 1);

Tab.

>          rqd->pick_bias = cpu;
> +    }
>  
>      return spc->runq_id;
>  }
> @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
>      __cpumask_clear_cpu(cpu, &rqd->smt_idle);
>      __cpumask_clear_cpu(cpu, &rqd->active);
>  
> -    if ( cpumask_empty(&rqd->active) )
> +    rqd->nr_cpus--;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
> +    if ( rqd->nr_cpus == 0 )
>      {
> +	ASSERT(cpumask_empty(&rqd->active));

And here.

Is it really worth having both asserts?  The second is redundant.

~Andrew

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

Re: [Xen-devel] [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Andrew Cooper]

On 20/04/2019 16:24, Dario Faggioli wrote:
> cpumask_weight() is known to be expensive. In Credit2, we use it in
> load-balancing, but only for knowing how many CPUs are active in a
> runqueue.

With a few small changes it can be improved substantially (by using
POPCNT when available), but it is still fundamentally an O(n) operation.

Definitely +1 to algorithm changes which avoid its use entirely.

A few cosmetic observations.

> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 6958b265fc..7034325243 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
>      spinlock_t lock;           /* Lock for this runqueue                     */
>  
>      struct list_head runq;     /* Ordered list of runnable vms               */
> +    int nr_cpus;               /* How many CPUs are sharing this runqueue    */

Unsigned int.

> @@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>                  __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
>              }
>  
> -    if ( cpumask_weight(&rqd->active) == 1 )
> +    if ( rqd->nr_cpus == 1 )
> +    {
> +	ASSERT(cpumask_weight(&rqd->active) == 1);

Tab.

>          rqd->pick_bias = cpu;
> +    }
>  
>      return spc->runq_id;
>  }
> @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
>      __cpumask_clear_cpu(cpu, &rqd->smt_idle);
>      __cpumask_clear_cpu(cpu, &rqd->active);
>  
> -    if ( cpumask_empty(&rqd->active) )
> +    rqd->nr_cpus--;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
> +    if ( rqd->nr_cpus == 0 )
>      {
> +	ASSERT(cpumask_empty(&rqd->active));

And here.

Is it really worth having both asserts?  The second is redundant.

~Andrew

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 23/04/2019 10:13, Andrii Anisov wrote:
> Hello Dario,
>
> On 20.04.19 18:24, Dario Faggioli wrote:
>> In schedule(), if we pick, as the next vcpu to run (next) the same one
>> that is running already (prev), we never get to call context_switch().
>
> And what about `if ( prev != current )` in arm/domain.c:schedule_tail() ?

schedule_tail() is called with current from the continue_running() path.

~Andrew

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 23/04/2019 10:13, Andrii Anisov wrote:
> Hello Dario,
>
> On 20.04.19 18:24, Dario Faggioli wrote:
>> In schedule(), if we pick, as the next vcpu to run (next) the same one
>> that is running already (prev), we never get to call context_switch().
>
> And what about `if ( prev != current )` in arm/domain.c:schedule_tail() ?

schedule_tail() is called with current from the continue_running() path.

~Andrew

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

Re: [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Andrii Anisov]

Hello Dario,

On 20.04.19 18:24, Dario Faggioli wrote:
> cpumask_weight() is known to be expensive. In Credit2, we use it in
> load-balancing, but only for knowing how many CPUs are active in a
> runqueue.
> 
> Keeping such count in an integer field of the per-runqueue data
> structure we have, completely avoids the need for cpumask_weight().
> 
> While there, remove as much other uses of it as we can, even if not in
> hot-paths.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>   xen/common/sched_credit2.c |   21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 6958b265fc..7034325243 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
>       spinlock_t lock;           /* Lock for this runqueue                     */
>   
>       struct list_head runq;     /* Ordered list of runnable vms               */
> +    int nr_cpus;               /* How many CPUs are sharing this runqueue    */
>       int id;                    /* ID of this runqueue (-1 if invalid)        */
>   
>       int load;                  /* Instantaneous load (num of non-idle vcpus) */
> @@ -2613,8 +2614,8 @@ retry:
>           if ( st.orqd->b_avgload > load_max )
>               load_max = st.orqd->b_avgload;
>   
> -        cpus_max = cpumask_weight(&st.lrqd->active);
> -        i = cpumask_weight(&st.orqd->active);
> +        cpus_max = st.lrqd->nr_cpus;
> +        i = st.orqd->nr_cpus;
>           if ( i > cpus_max )
>               cpus_max = i;
>   
> @@ -3697,7 +3698,7 @@ csched2_dump(const struct scheduler *ops)
>                  "\tinstload           = %d\n"
>                  "\taveload            = %"PRI_stime" (~%"PRI_stime"%%)\n",
>                  i,
> -               cpumask_weight(&prv->rqd[i].active),
> +               prv->rqd[i].nr_cpus,
>                  nr_cpu_ids, cpumask_bits(&prv->rqd[i].active),
>                  prv->rqd[i].max_weight,
>                  prv->rqd[i].pick_bias,
> @@ -3818,6 +3819,9 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>       __cpumask_set_cpu(cpu, &prv->initialized);
>       __cpumask_set_cpu(cpu, &rqd->smt_idle);
>   
> +    rqd->nr_cpus++;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
>       /* On the boot cpu we are called before cpu_sibling_mask has been set up. */
>       if ( cpu == 0 && system_state < SYS_STATE_active )
>           __cpumask_set_cpu(cpu, &csched2_pcpu(cpu)->sibling_mask);
> @@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>                   __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
>               }
>   
> -    if ( cpumask_weight(&rqd->active) == 1 )
> +    if ( rqd->nr_cpus == 1 )
> +    {
> +	ASSERT(cpumask_weight(&rqd->active) == 1);

Please do not use hard tabs.

>           rqd->pick_bias = cpu;
> +    }
>   
>       return spc->runq_id;
>   }
> @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
>       __cpumask_clear_cpu(cpu, &rqd->smt_idle);
>       __cpumask_clear_cpu(cpu, &rqd->active);
>   
> -    if ( cpumask_empty(&rqd->active) )
> +    rqd->nr_cpus--;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
> +    if ( rqd->nr_cpus == 0 )
>       {
> +	ASSERT(cpumask_empty(&rqd->active));

Ditto.

>           printk(XENLOG_INFO " No cpus left on runqueue, disabling\n");
>           deactivate_runqueue(prv, spc->runq_id);
>       }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

With nits fixed:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Andrii Anisov]

Hello Dario,

On 20.04.19 18:24, Dario Faggioli wrote:
> cpumask_weight() is known to be expensive. In Credit2, we use it in
> load-balancing, but only for knowing how many CPUs are active in a
> runqueue.
> 
> Keeping such count in an integer field of the per-runqueue data
> structure we have, completely avoids the need for cpumask_weight().
> 
> While there, remove as much other uses of it as we can, even if not in
> hot-paths.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>   xen/common/sched_credit2.c |   21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 6958b265fc..7034325243 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
>       spinlock_t lock;           /* Lock for this runqueue                     */
>   
>       struct list_head runq;     /* Ordered list of runnable vms               */
> +    int nr_cpus;               /* How many CPUs are sharing this runqueue    */
>       int id;                    /* ID of this runqueue (-1 if invalid)        */
>   
>       int load;                  /* Instantaneous load (num of non-idle vcpus) */
> @@ -2613,8 +2614,8 @@ retry:
>           if ( st.orqd->b_avgload > load_max )
>               load_max = st.orqd->b_avgload;
>   
> -        cpus_max = cpumask_weight(&st.lrqd->active);
> -        i = cpumask_weight(&st.orqd->active);
> +        cpus_max = st.lrqd->nr_cpus;
> +        i = st.orqd->nr_cpus;
>           if ( i > cpus_max )
>               cpus_max = i;
>   
> @@ -3697,7 +3698,7 @@ csched2_dump(const struct scheduler *ops)
>                  "\tinstload           = %d\n"
>                  "\taveload            = %"PRI_stime" (~%"PRI_stime"%%)\n",
>                  i,
> -               cpumask_weight(&prv->rqd[i].active),
> +               prv->rqd[i].nr_cpus,
>                  nr_cpu_ids, cpumask_bits(&prv->rqd[i].active),
>                  prv->rqd[i].max_weight,
>                  prv->rqd[i].pick_bias,
> @@ -3818,6 +3819,9 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>       __cpumask_set_cpu(cpu, &prv->initialized);
>       __cpumask_set_cpu(cpu, &rqd->smt_idle);
>   
> +    rqd->nr_cpus++;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
>       /* On the boot cpu we are called before cpu_sibling_mask has been set up. */
>       if ( cpu == 0 && system_state < SYS_STATE_active )
>           __cpumask_set_cpu(cpu, &csched2_pcpu(cpu)->sibling_mask);
> @@ -3829,8 +3833,11 @@ init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
>                   __cpumask_set_cpu(rcpu, &csched2_pcpu(cpu)->sibling_mask);
>               }
>   
> -    if ( cpumask_weight(&rqd->active) == 1 )
> +    if ( rqd->nr_cpus == 1 )
> +    {
> +	ASSERT(cpumask_weight(&rqd->active) == 1);

Please do not use hard tabs.

>           rqd->pick_bias = cpu;
> +    }
>   
>       return spc->runq_id;
>   }
> @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
>       __cpumask_clear_cpu(cpu, &rqd->smt_idle);
>       __cpumask_clear_cpu(cpu, &rqd->active);
>   
> -    if ( cpumask_empty(&rqd->active) )
> +    rqd->nr_cpus--;
> +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> +
> +    if ( rqd->nr_cpus == 0 )
>       {
> +	ASSERT(cpumask_empty(&rqd->active));

Ditto.

>           printk(XENLOG_INFO " No cpus left on runqueue, disabling\n");
>           deactivate_runqueue(prv, spc->runq_id);
>       }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

With nits fixed:

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[George Dunlap]

> On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)

Keeping in mind that ASSERT() is merely a debugging aid: suppose that testing didn’t discover this, and a bug that violated this assumption slipped into production.  Would the patched code DTRT?

It sort of looks like the prev/next checking is an optimization, and that duplicate checking shouldn’t cause any problems.  Is that the case?

 -George

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[George Dunlap]

> On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
> 
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)

Keeping in mind that ASSERT() is merely a debugging aid: suppose that testing didn’t discover this, and a bug that violated this assumption slipped into production.  Would the patched code DTRT?

It sort of looks like the prev/next checking is an optimization, and that duplicate checking shouldn’t cause any problems.  Is that the case?

 -George

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Juergen Gross]

On 23/04/2019 11:50, George Dunlap wrote:
> 
> 
>> On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com> wrote:
>>
>> In schedule(), if we pick, as the next vcpu to run (next) the same one
>> that is running already (prev), we never get to call context_switch().
>>
>> We can, therefore, get rid of all the `if`-s testing prev and next being
>> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
>> already there!)
> 
> Keeping in mind that ASSERT() is merely a debugging aid: suppose that testing didn’t discover this, and a bug that violated this assumption slipped into production.  Would the patched code DTRT?

The unpatched code would already do something wrong: it would clear
v->is_running (through context_saved()) for the running vcpu,
potentially leading to all sorts of wrong decisions in the scheduling
code later.


Juergen

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Juergen Gross]

On 23/04/2019 11:50, George Dunlap wrote:
> 
> 
>> On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com> wrote:
>>
>> In schedule(), if we pick, as the next vcpu to run (next) the same one
>> that is running already (prev), we never get to call context_switch().
>>
>> We can, therefore, get rid of all the `if`-s testing prev and next being
>> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
>> already there!)
> 
> Keeping in mind that ASSERT() is merely a debugging aid: suppose that testing didn’t discover this, and a bug that violated this assumption slipped into production.  Would the patched code DTRT?

The unpatched code would already do something wrong: it would clear
v->is_running (through context_saved()) for the running vcpu,
potentially leading to all sorts of wrong decisions in the scheduling
code later.


Juergen

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 20/04/2019 16:24, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
>
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)
>
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll pull this into x86-next given a complete set of R/A-by's

~Andrew

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 20/04/2019 16:24, Dario Faggioli wrote:
> In schedule(), if we pick, as the next vcpu to run (next) the same one
> that is running already (prev), we never get to call context_switch().
>
> We can, therefore, get rid of all the `if`-s testing prev and next being
> different, trading them with an ASSERT() (on ARM, the ASSERT() was even
> already there!)
>
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Suggested-by: Juergen Gross <jgross@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll pull this into x86-next given a complete set of R/A-by's

~Andrew

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Andrew,

On 23.04.19 12:47, Andrew Cooper wrote:
> schedule_tail() is called with current from the continue_running() path.
Not on ARM. On ARM continue_running() is empty.
For the quick check I've put a BUG() into the `else` branch, and did not hit it during system up, domain create/destroy, etc.

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Andrew,

On 23.04.19 12:47, Andrew Cooper wrote:
> schedule_tail() is called with current from the continue_running() path.
Not on ARM. On ARM continue_running() is empty.
For the quick check I've put a BUG() into the `else` branch, and did not hit it during system up, domain create/destroy, etc.

-- 
Sincerely,
Andrii Anisov.

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

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

On Tue, 2019-04-23 at 11:56 +0200, Juergen Gross wrote:
> On 23/04/2019 11:50, George Dunlap wrote:
> > 
> > > On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com>
> > > wrote:
> > > 
> > > We can, therefore, get rid of all the `if`-s testing prev and
> > > next being
> > > different, trading them with an ASSERT() (on ARM, the ASSERT()
> > > was even
> > > already there!)
> > 
> > Keeping in mind that ASSERT() is merely a debugging aid: suppose
> > that testing didn’t discover this, and a bug that violated this
> > assumption slipped into production.  Would the patched code DTRT?
> 
> The unpatched code would already do something wrong: it would clear
> v->is_running (through context_saved()) for the running vcpu,
> potentially leading to all sorts of wrong decisions in the scheduling
> code later.
> 
Exactly! Which basically means it's quite likely that, if we ever get
past the call to:

  return continue_running(prev)  (in schedule.c:schedule())

with next==prev, things will go very wrong, without the `if`-s that
this patch is killing being of much help.

And, yes, this is (micro) optimizing, but it's also an attempt to
improve the code, making it clear, for people paying with
context_switch(), that they don't have to deal with the special case of
prev and next being equal, while right now they may think they do.

Even if we decide to keep the `if`-s, I'd still like to have the ASSERT
() (and a comment explaining why we have both the assertion and the
checks).

I haven't done archaeology to find out when it was (if ever) that it
was ok to call context_switch() with next==prev. I can, and see about
adding what I find in the changelog, if you (George) think it could be
interesting.

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

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

On Tue, 2019-04-23 at 11:56 +0200, Juergen Gross wrote:
> On 23/04/2019 11:50, George Dunlap wrote:
> > 
> > > On Apr 20, 2019, at 4:24 PM, Dario Faggioli <dfaggioli@suse.com>
> > > wrote:
> > > 
> > > We can, therefore, get rid of all the `if`-s testing prev and
> > > next being
> > > different, trading them with an ASSERT() (on ARM, the ASSERT()
> > > was even
> > > already there!)
> > 
> > Keeping in mind that ASSERT() is merely a debugging aid: suppose
> > that testing didn’t discover this, and a bug that violated this
> > assumption slipped into production.  Would the patched code DTRT?
> 
> The unpatched code would already do something wrong: it would clear
> v->is_running (through context_saved()) for the running vcpu,
> potentially leading to all sorts of wrong decisions in the scheduling
> code later.
> 
Exactly! Which basically means it's quite likely that, if we ever get
past the call to:

  return continue_running(prev)  (in schedule.c:schedule())

with next==prev, things will go very wrong, without the `if`-s that
this patch is killing being of much help.

And, yes, this is (micro) optimizing, but it's also an attempt to
improve the code, making it clear, for people paying with
context_switch(), that they don't have to deal with the special case of
prev and next being equal, while right now they may think they do.

Even if we decide to keep the `if`-s, I'd still like to have the ASSERT
() (and a comment explaining why we have both the assertion and the
checks).

I haven't done archaeology to find out when it was (if ever) that it
was ok to call context_switch() with next==prev. I can, and see about
adding what I find in the changelog, if you (George) think it could be
interesting.

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

Re: [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Dario Faggioli]

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

On Tue, 2019-04-23 at 10:44 +0100, Andrew Cooper wrote:
> On 20/04/2019 16:24, Dario Faggioli wrote:
>
> Definitely +1 to algorithm changes which avoid its use entirely.
> 
> A few cosmetic observations.
> 
Ok...

> > diff --git a/xen/common/sched_credit2.c
> > b/xen/common/sched_credit2.c
> > index 6958b265fc..7034325243 100644
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
> >      spinlock_t lock;           /* Lock for this
> > runqueue                     */
> >  
> >      struct list_head runq;     /* Ordered list of runnable
> > vms               */
> > +    int nr_cpus;               /* How many CPUs are sharing this
> > runqueue    */
> 
> Unsigned int.
> 
Yep.

> > @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler
> > *ops, void *pcpu, int cpu)
> >      __cpumask_clear_cpu(cpu, &rqd->smt_idle);
> >      __cpumask_clear_cpu(cpu, &rqd->active);
> >  
> > -    if ( cpumask_empty(&rqd->active) )
> > +    rqd->nr_cpus--;
> > +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> > +
> > +    if ( rqd->nr_cpus == 0 )
> >      {
> > +	ASSERT(cpumask_empty(&rqd->active));
> 
> And here.
> 
Right. Not sure how they ended in there. Will remove them.

> Is it really worth having both asserts?  The second is redundant.
> 
I think I agree. I'll keep the first one only.

Thanks and 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

Re: [Xen-devel] [PATCH 1/2] xen: credit2: avoid using cpumask_weight() in hot-paths

[Dario Faggioli]

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

On Tue, 2019-04-23 at 10:44 +0100, Andrew Cooper wrote:
> On 20/04/2019 16:24, Dario Faggioli wrote:
>
> Definitely +1 to algorithm changes which avoid its use entirely.
> 
> A few cosmetic observations.
> 
Ok...

> > diff --git a/xen/common/sched_credit2.c
> > b/xen/common/sched_credit2.c
> > index 6958b265fc..7034325243 100644
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
> >      spinlock_t lock;           /* Lock for this
> > runqueue                     */
> >  
> >      struct list_head runq;     /* Ordered list of runnable
> > vms               */
> > +    int nr_cpus;               /* How many CPUs are sharing this
> > runqueue    */
> 
> Unsigned int.
> 
Yep.

> > @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler
> > *ops, void *pcpu, int cpu)
> >      __cpumask_clear_cpu(cpu, &rqd->smt_idle);
> >      __cpumask_clear_cpu(cpu, &rqd->active);
> >  
> > -    if ( cpumask_empty(&rqd->active) )
> > +    rqd->nr_cpus--;
> > +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> > +
> > +    if ( rqd->nr_cpus == 0 )
> >      {
> > +	ASSERT(cpumask_empty(&rqd->active));
> 
> And here.
> 
Right. Not sure how they ended in there. Will remove them.

> Is it really worth having both asserts?  The second is redundant.
> 
I think I agree. I'll keep the first one only.

Thanks and 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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

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

On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
> Hello Dario,
> 
> On 20.04.19 18:24, Dario Faggioli wrote:
> > In schedule(), if we pick, as the next vcpu to run (next) the same
> > one
> > that is running already (prev), we never get to call
> > context_switch().
> 
> And what about `if ( prev != current )` in
> arm/domain.c:schedule_tail() ?
> 
You're suggesting that's redundant too, aren't you?

From a quick look, it seems to me as well that it would be. I'm ok
taking care of it, not sure if in this patch, or in another one.

I'm open to suggestions from ARM maintainers...

Thanks and 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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Dario Faggioli]

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

On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
> Hello Dario,
> 
> On 20.04.19 18:24, Dario Faggioli wrote:
> > In schedule(), if we pick, as the next vcpu to run (next) the same
> > one
> > that is running already (prev), we never get to call
> > context_switch().
> 
> And what about `if ( prev != current )` in
> arm/domain.c:schedule_tail() ?
> 
You're suggesting that's redundant too, aren't you?

From a quick look, it seems to me as well that it would be. I'm ok
taking care of it, not sure if in this patch, or in another one.

I'm open to suggestions from ARM maintainers...

Thanks and 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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Julien Grall]

Hi Dario,

On 26/04/2019 16:13, Dario Faggioli wrote:
> On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
>> Hello Dario,
>>
>> On 20.04.19 18:24, Dario Faggioli wrote:
>>> In schedule(), if we pick, as the next vcpu to run (next) the same
>>> one
>>> that is running already (prev), we never get to call
>>> context_switch().
>>
>> And what about `if ( prev != current )` in
>> arm/domain.c:schedule_tail() ?
>>
> You're suggesting that's redundant too, aren't you?
> 
>  From a quick look, it seems to me as well that it would be. I'm ok
> taking care of it, not sure if in this patch, or in another one.

I thought Andrew already queued this patch?

Anyway, I think you are right. We can drop the check in schedule_tail() as well.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Julien Grall]

Hi Dario,

On 26/04/2019 16:13, Dario Faggioli wrote:
> On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
>> Hello Dario,
>>
>> On 20.04.19 18:24, Dario Faggioli wrote:
>>> In schedule(), if we pick, as the next vcpu to run (next) the same
>>> one
>>> that is running already (prev), we never get to call
>>> context_switch().
>>
>> And what about `if ( prev != current )` in
>> arm/domain.c:schedule_tail() ?
>>
> You're suggesting that's redundant too, aren't you?
> 
>  From a quick look, it seems to me as well that it would be. I'm ok
> taking care of it, not sure if in this patch, or in another one.

I thought Andrew already queued this patch?

Anyway, I think you are right. We can drop the check in schedule_tail() as well.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 26/04/2019 17:22, Julien Grall wrote:
> Hi Dario,
>
> On 26/04/2019 16:13, Dario Faggioli wrote:
>> On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
>>> Hello Dario,
>>>
>>> On 20.04.19 18:24, Dario Faggioli wrote:
>>>> In schedule(), if we pick, as the next vcpu to run (next) the same
>>>> one
>>>> that is running already (prev), we never get to call
>>>> context_switch().
>>>
>>> And what about `if ( prev != current )` in
>>> arm/domain.c:schedule_tail() ?
>>>
>> You're suggesting that's redundant too, aren't you?
>>
>>  From a quick look, it seems to me as well that it would be. I'm ok
>> taking care of it, not sure if in this patch, or in another one.
>
> I thought Andrew already queued this patch?

I did.

>
> Anyway, I think you are right. We can drop the check in
> schedule_tail() as well.

That should really be a separate patch, because it is ARM-specific
scheduling behaviour.  The same is not true on x86.

~Andrew

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrew Cooper]

On 26/04/2019 17:22, Julien Grall wrote:
> Hi Dario,
>
> On 26/04/2019 16:13, Dario Faggioli wrote:
>> On Tue, 2019-04-23 at 12:13 +0300, Andrii Anisov wrote:
>>> Hello Dario,
>>>
>>> On 20.04.19 18:24, Dario Faggioli wrote:
>>>> In schedule(), if we pick, as the next vcpu to run (next) the same
>>>> one
>>>> that is running already (prev), we never get to call
>>>> context_switch().
>>>
>>> And what about `if ( prev != current )` in
>>> arm/domain.c:schedule_tail() ?
>>>
>> You're suggesting that's redundant too, aren't you?
>>
>>  From a quick look, it seems to me as well that it would be. I'm ok
>> taking care of it, not sure if in this patch, or in another one.
>
> I thought Andrew already queued this patch?

I did.

>
> Anyway, I think you are right. We can drop the check in
> schedule_tail() as well.

That should really be a separate patch, because it is ARM-specific
scheduling behaviour.  The same is not true on x86.

~Andrew

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

Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Dario,

Sorry for a delayed answer,

On 26.04.19 18:13, Dario Faggioli wrote:
> You're suggesting that's redundant too, aren't you?

Yes, I do.
And have pushed the patch [1].

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg00597.html

-- 
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next

[Andrii Anisov]

Hello Dario,

Sorry for a delayed answer,

On 26.04.19 18:13, Dario Faggioli wrote:
> You're suggesting that's redundant too, aren't you?

Yes, I do.
And have pushed the patch [1].

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg00597.html

-- 
Sincerely,
Andrii Anisov.

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