* [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-20 15:24 ` Dario Faggioli
0 siblings, 0 replies; 36+ messages in thread
From: Dario Faggioli @ 2019-04-20 15:24 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-21 17:08 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2019-04-21 17:08 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-21 17:08 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2019-04-21 17:08 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:13 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-04-23 9:13 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:13 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-04-23 9:13 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:47 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-23 9:47 UTC (permalink / raw)
To: Andrii Anisov, Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:47 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-23 9:47 UTC (permalink / raw)
To: Andrii Anisov, Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 10:33 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-04-23 10:33 UTC (permalink / raw)
To: Andrew Cooper, Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 10:33 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-04-23 10:33 UTC (permalink / raw)
To: Andrew Cooper, Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 15:13 ` Dario Faggioli
0 siblings, 0 replies; 36+ messages in thread
From: Dario Faggioli @ 2019-04-26 15:13 UTC (permalink / raw)
To: Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 15:13 ` Dario Faggioli
0 siblings, 0 replies; 36+ messages in thread
From: Dario Faggioli @ 2019-04-26 15:13 UTC (permalink / raw)
To: Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 16:22 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2019-04-26 16:22 UTC (permalink / raw)
To: Dario Faggioli, Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 16:22 ` Julien Grall
0 siblings, 0 replies; 36+ messages in thread
From: Julien Grall @ 2019-04-26 16:22 UTC (permalink / raw)
To: Dario Faggioli, Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 16:26 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-26 16:26 UTC (permalink / raw)
To: Julien Grall, Dario Faggioli, Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 16:26 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-26 16:26 UTC (permalink / raw)
To: Julien Grall, Dario Faggioli, Andrii Anisov, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-05-08 10:03 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-05-08 10:03 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-05-08 10:03 ` Andrii Anisov
0 siblings, 0 replies; 36+ messages in thread
From: Andrii Anisov @ 2019-05-08 10:03 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:50 ` George Dunlap
0 siblings, 0 replies; 36+ messages in thread
From: George Dunlap @ 2019-04-23 9:50 UTC (permalink / raw)
To: Dario Faggioli
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
Julien Grall, Jan Beulich, Xen-devel, Roger Pau Monne
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:50 ` George Dunlap
0 siblings, 0 replies; 36+ messages in thread
From: George Dunlap @ 2019-04-23 9:50 UTC (permalink / raw)
To: Dario Faggioli
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper,
Julien Grall, Jan Beulich, Xen-devel, Roger Pau Monne
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:56 ` Juergen Gross
0 siblings, 0 replies; 36+ messages in thread
From: Juergen Gross @ 2019-04-23 9:56 UTC (permalink / raw)
To: George Dunlap, Dario Faggioli
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Xen-devel, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 9:56 ` Juergen Gross
0 siblings, 0 replies; 36+ messages in thread
From: Juergen Gross @ 2019-04-23 9:56 UTC (permalink / raw)
To: George Dunlap, Dario Faggioli
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Xen-devel, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 12:58 ` Dario Faggioli
0 siblings, 0 replies; 36+ messages in thread
From: Dario Faggioli @ 2019-04-26 12:58 UTC (permalink / raw)
To: Juergen Gross, George Dunlap
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Xen-devel, Roger Pau Monne
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-26 12:58 ` Dario Faggioli
0 siblings, 0 replies; 36+ messages in thread
From: Dario Faggioli @ 2019-04-26 12:58 UTC (permalink / raw)
To: Juergen Gross, George Dunlap
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Xen-devel, Roger Pau Monne
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 10:00 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-23 10:00 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen: sched: we never get into context_switch() with prev==next
@ 2019-04-23 10:00 ` Andrew Cooper
0 siblings, 0 replies; 36+ messages in thread
From: Andrew Cooper @ 2019-04-23 10:00 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu, George Dunlap,
Julien Grall, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 36+ messages in thread