| Message ID | 20210119003206.2255-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/domain: Introduce vcpu_teardown() | expand |
On 19.01.2021 01:32, Andrew Cooper wrote: > Similarly to c/s 98d4d6d8a6 "xen/domain: Introduce domain_teardown()", > introduce a common mechanism for restartable per-vcpu teardown logic. > > Extend the PROGRESS() mechanism to support saving and restoring the vcpu loop > variable across hypercalls. > > This will eventually supersede domain_reliquish_resources(), and reduce the > quantity of redundant logic performed. > > No functional change (yet). > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit perhaps with a name or type change: > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -532,6 +532,7 @@ struct domain > */ > struct { > unsigned int val; > + struct vcpu *ptr; > } teardown; I think the field either wants to be generic (and then of type void *) or specific (and then be named "vcpu"). Which one is better certainly depends on possibly anticipated future usage. Jan
On 19/01/2021 09:14, Jan Beulich wrote: > On 19.01.2021 01:32, Andrew Cooper wrote: >> Similarly to c/s 98d4d6d8a6 "xen/domain: Introduce domain_teardown()", >> introduce a common mechanism for restartable per-vcpu teardown logic. >> >> Extend the PROGRESS() mechanism to support saving and restoring the vcpu loop >> variable across hypercalls. >> >> This will eventually supersede domain_reliquish_resources(), and reduce the >> quantity of redundant logic performed. >> >> No functional change (yet). >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > albeit perhaps with a name or type change: > >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -532,6 +532,7 @@ struct domain >> */ >> struct { >> unsigned int val; >> + struct vcpu *ptr; >> } teardown; > I think the field either wants to be generic (and then of type void *) > or specific (and then be named "vcpu"). Which one is better certainly > depends on possibly anticipated future usage. I think I'll rename to vcpu for now. I don't anticipate this being usable for anything else safely. ~Andrew
diff --git a/xen/common/domain.c b/xen/common/domain.c index 164c9d14e9..7be3a7cf36 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -130,6 +130,22 @@ static void vcpu_info_reset(struct vcpu *v) v->vcpu_info_mfn = INVALID_MFN; } +/* + * Release resources held by a vcpu. There may or may not be live references + * to the vcpu, and it may or may not be fully constructed. + * + * If d->is_dying is DOMDYING_dead, this must not return non-zero. + */ +static int vcpu_teardown(struct vcpu *v) +{ + return 0; +} + +/* + * Destoy a vcpu once all references to it have been dropped. Used either + * from domain_destroy()'s RCU path, or from the vcpu_create() error path + * before the vcpu is placed on the domain's vcpu list. + */ static void vcpu_destroy(struct vcpu *v) { free_vcpu_struct(v); @@ -206,6 +222,11 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id) sched_destroy_vcpu(v); fail_wq: destroy_waitqueue_vcpu(v); + + /* Must not hit a continuation in this context. */ + if ( vcpu_teardown(v) ) + ASSERT_UNREACHABLE(); + vcpu_destroy(v); return NULL; @@ -284,6 +305,9 @@ custom_param("extra_guest_irqs", parse_extra_guest_irqs); */ static int domain_teardown(struct domain *d) { + struct vcpu *v; + int rc; + BUG_ON(!d->is_dying); /* @@ -298,7 +322,9 @@ static int domain_teardown(struct domain *d) * will logically restart work from this point. * * PROGRESS() markers must not be in the middle of loops. The loop - * variable isn't preserved across a continuation. + * variable isn't preserved across a continuation. PROGRESS_VCPU() + * markers may be used in the middle of for_each_vcpu() loops, which + * preserve v but no other loop variables. * * To avoid redundant work, there should be a marker before each * function which may return -ERESTART. @@ -308,14 +334,32 @@ static int domain_teardown(struct domain *d) /* Fallthrough */ \ case PROG_ ## x +#define PROGRESS_VCPU(x) \ + d->teardown.val = PROG_vcpu_ ## x; \ + d->teardown.ptr = v; \ + /* Fallthrough */ \ + case PROG_vcpu_ ## x: \ + v = d->teardown.ptr + enum { - PROG_done = 1, + PROG_vcpu_teardown = 1, + PROG_done, }; case 0: + for_each_vcpu ( d, v ) + { + PROGRESS_VCPU(teardown); + + rc = vcpu_teardown(v); + if ( rc ) + return rc; + } + PROGRESS(done): break; +#undef PROGRESS_VCPU #undef PROGRESS default: diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 3e46384a3c..846a77c0bb 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -532,6 +532,7 @@ struct domain */ struct { unsigned int val; + struct vcpu *ptr; } teardown; };
Similarly to c/s 98d4d6d8a6 "xen/domain: Introduce domain_teardown()", introduce a common mechanism for restartable per-vcpu teardown logic. Extend the PROGRESS() mechanism to support saving and restoring the vcpu loop variable across hypercalls. This will eventually supersede domain_reliquish_resources(), and reduce the quantity of redundant logic performed. No functional change (yet). 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: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> This is a prerequisite for the IPT series, to avoid introducing a latent memory leak bug on ARM. --- xen/common/domain.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- xen/include/xen/sched.h | 1 + 2 files changed, 47 insertions(+), 2 deletions(-)