| Message ID | 20220718071825.22113-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/wait: Improvements | expand |
On 18.07.2022 09:18, Andrew Cooper wrote: > With the waitqueue logic updated to not use an absolute stack pointer > reference, the vCPU can safely be resumed anywhere. > > Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes, I understand you mean two domain_crash() invocations here, but ... > and a > logical corner case where resetting the vcpu with an oustanding waitqueue > would crash the domain. ... some other domain crash here? > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I assume you've checked thoroughly that calling code hasn't grown dependencies on execution coming back on the same CPU? Jan
On 18/07/2022 11:48, Jan Beulich wrote: > On 18.07.2022 09:18, Andrew Cooper wrote: >> With the waitqueue logic updated to not use an absolute stack pointer >> reference, the vCPU can safely be resumed anywhere. >> >> Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes, > I understand you mean two domain_crash() invocations here, but ... > >> and a >> logical corner case where resetting the vcpu with an oustanding waitqueue >> would crash the domain. > ... some other domain crash here? One of the two above. It's more that resetting (would have) broken the affinity and would have triggered the domain crash. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I assume you've checked thoroughly that calling code hasn't > grown dependencies on execution coming back on the same CPU? Urgh yes, my trivial test case didn't encounter it, but anything with an smp_processor_id() stashed on the stack is going to end up unhappy. I'm going to have to retract half this series. (I'll follow up on the 0/$N with the longer term plan to remove this mess). ~Andrew
diff --git a/xen/common/domain.c b/xen/common/domain.c index 618410e3b257..323b92102cce 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1428,8 +1428,6 @@ int vcpu_reset(struct vcpu *v) v->is_initialised = 0; if ( v->affinity_broken & VCPU_AFFINITY_OVERRIDE ) vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_OVERRIDE); - if ( v->affinity_broken & VCPU_AFFINITY_WAIT ) - vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_WAIT); clear_bit(_VPF_blocked, &v->pause_flags); clear_bit(_VPF_in_reset, &v->pause_flags); diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index f689b55783f7..cff8e59aba7c 100644 --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -1610,12 +1610,10 @@ void watchdog_domain_destroy(struct domain *d) /* * Pin a vcpu temporarily to a specific CPU (or restore old pinning state if * cpu is NR_CPUS). - * Temporary pinning can be done due to two reasons, which may be nested: + * Temporary pinning can be done for a number of reasons, which may be nested: * - VCPU_AFFINITY_OVERRIDE (requested by guest): is allowed to fail in case * of a conflict (e.g. in case cpupool doesn't include requested CPU, or * another conflicting temporary pinning is already in effect. - * - VCPU_AFFINITY_WAIT (called by wait_event()): only used to pin vcpu to the - * CPU it is just running on. Can't fail if used properly. */ int vcpu_temporary_affinity(struct vcpu *v, unsigned int cpu, uint8_t reason) { diff --git a/xen/common/wait.c b/xen/common/wait.c index 4f1daf650bc4..bd6f09662ac0 100644 --- a/xen/common/wait.c +++ b/xen/common/wait.c @@ -127,16 +127,6 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv) ASSERT(wqv->used == 0); - /* Save current VCPU affinity; force wakeup on *this* CPU only. */ - if ( vcpu_temporary_affinity(curr, smp_processor_id(), VCPU_AFFINITY_WAIT) ) - { - gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n"); - domain_crash(curr->domain); - - for ( ; ; ) - do_softirq(); - } - /* * Hand-rolled setjmp(). * @@ -187,7 +177,6 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv) static void __finish_wait(struct waitqueue_vcpu *wqv) { wqv->used = 0; - vcpu_temporary_affinity(current, NR_CPUS, VCPU_AFFINITY_WAIT); } void check_wakeup_from_wait(void) @@ -201,18 +190,6 @@ void check_wakeup_from_wait(void) if ( likely(!wqv->used) ) return; - /* Check if we are still pinned. */ - if ( unlikely(!(curr->affinity_broken & VCPU_AFFINITY_WAIT)) ) - { - gdprintk(XENLOG_ERR, "vcpu affinity lost\n"); - domain_crash(curr->domain); - - /* Re-initiate scheduler and don't longjmp(). */ - raise_softirq(SCHEDULE_SOFTIRQ); - for ( ; ; ) - do_softirq(); - } - /* * Hand-rolled longjmp(). * diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index b9515eb497de..ba859a4abed3 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -223,7 +223,6 @@ struct vcpu /* VCPU need affinity restored */ uint8_t affinity_broken; #define VCPU_AFFINITY_OVERRIDE 0x01 -#define VCPU_AFFINITY_WAIT 0x02 /* A hypercall has been preempted. */ bool hcall_preempted;
With the waitqueue logic updated to not use an absolute stack pointer reference, the vCPU can safely be resumed anywhere. Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes, and a logical corner case where resetting the vcpu with an oustanding waitqueue would crash the domain. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Juergen Gross <jgross@suse.com> CC: Dario Faggioli <dfaggioli@suse.com> --- xen/common/domain.c | 2 -- xen/common/sched/core.c | 4 +--- xen/common/wait.c | 23 ----------------------- xen/include/xen/sched.h | 1 - 4 files changed, 1 insertion(+), 29 deletions(-)