| Message ID | eb85d7fee920b54eea3b4c0e77ab40593613ccc4.1586270820.git.apanyaki@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] libxc/migration: Abort migration on precopy policy request | expand |
On Tue, Apr 07, 2020 at 02:52:22PM +0000, Andrew Panyakin wrote: > libxc defines XGS_POLICY_ABORT for precopy policy to signal that migration > should be aborted (eg. if the estimated pause time is too huge for the > instance). Default simple precopy policy never returns that, but it could be > overriden with a custom one. > Right. I think this is a real problem. > Signed-off-by: Andrew Panyakin <apanyaki@amazon.com> > --- > tools/libxc/xc_sr_save.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c > index fa736a311f..507274ce22 100644 > --- a/tools/libxc/xc_sr_save.c > +++ b/tools/libxc/xc_sr_save.c > @@ -560,6 +560,12 @@ static int send_memory_live(struct xc_sr_context *ctx) > > } > > + if ( policy_decision == XGS_POLICY_ABORT ) { The { should be on a new line. > + PERROR("Abort precopy loop"); > + rc = -1; > + goto out; There is no need to have "goto out" here. These can be fixed easily while committing, so no need to resend yet. I will give other people a chance to comment. Wei. > + } > + > out: > xc_set_progress_prefix(xch, NULL); > free(progress_str); > -- > 2.16.6 >
On 4/7/20 10:22 PM, Wei Liu wrote: > On Tue, Apr 07, 2020 at 02:52:22PM +0000, Andrew Panyakin wrote: >> libxc defines XGS_POLICY_ABORT for precopy policy to signal that migration >> should be aborted (eg. if the estimated pause time is too huge for the >> instance). Default simple precopy policy never returns that, but it could be >> overriden with a custom one. >> > > Right. I think this is a real problem. > >> Signed-off-by: Andrew Panyakin <apanyaki@amazon.com> >> --- >> tools/libxc/xc_sr_save.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >> index fa736a311f..507274ce22 100644 >> --- a/tools/libxc/xc_sr_save.c >> +++ b/tools/libxc/xc_sr_save.c >> @@ -560,6 +560,12 @@ static int send_memory_live(struct xc_sr_context *ctx) >> >> } >> >> + if ( policy_decision == XGS_POLICY_ABORT ) { > > The { should be on a new line. > >> + PERROR("Abort precopy loop"); >> + rc = -1; >> + goto out; > > There is no need to have "goto out" here. I was considering two more examples of "goto out" in a branch right before the label: - send_domain_memory_nonlive, - send_domain_memory_live. Isn't it done this way to simplify the function extension: you won't need to add "goto out" to previous branch when adding new code? > > These can be fixed easily while committing, so no need to resend yet. I > will give other people a chance to comment. > > Wei. > >> + } >> + >> out: >> xc_set_progress_prefix(xch, NULL); >> free(progress_str); >> -- >> 2.16.6 >>
On 07/04/2020 23:06, Panyakin, Andrew wrote: > On 4/7/20 10:22 PM, Wei Liu wrote: >>> + PERROR("Abort precopy loop"); >>> + rc = -1; >>> + goto out; >> There is no need to have "goto out" here. > I was considering two more examples of "goto out" in a branch right before the label: > - send_domain_memory_nonlive, > - send_domain_memory_live. > > Isn't it done this way to simplify the function extension: you won't need to add "goto out" to previous branch when adding new code? I'd recommend leaving the goto out like this. Less effort for the next person editing this code to think about. >> These can be fixed easily while committing, so no need to resend yet. I >> will give other people a chance to comment. None of the copy policy was done well. If Amazon have a usecase then lets put it in. (Talking of - I wonder why XenServer's usecase hasn't tripped over this... This was put into to help negotiate two live streams at once, but this is an error case which surely ought to trigger.) ~Andrew
On Wed, Apr 08, 2020 at 12:06:22AM +0200, Panyakin, Andrew wrote: > On 4/7/20 10:22 PM, Wei Liu wrote: > > On Tue, Apr 07, 2020 at 02:52:22PM +0000, Andrew Panyakin wrote: > >> libxc defines XGS_POLICY_ABORT for precopy policy to signal that migration > >> should be aborted (eg. if the estimated pause time is too huge for the > >> instance). Default simple precopy policy never returns that, but it could be > >> overriden with a custom one. > >> > > > > Right. I think this is a real problem. > > > >> Signed-off-by: Andrew Panyakin <apanyaki@amazon.com> > >> --- > >> tools/libxc/xc_sr_save.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c > >> index fa736a311f..507274ce22 100644 > >> --- a/tools/libxc/xc_sr_save.c > >> +++ b/tools/libxc/xc_sr_save.c > >> @@ -560,6 +560,12 @@ static int send_memory_live(struct xc_sr_context *ctx) > >> > >> } > >> > >> + if ( policy_decision == XGS_POLICY_ABORT ) { > > > > The { should be on a new line. > > > >> + PERROR("Abort precopy loop"); > >> + rc = -1; > >> + goto out; > > > > There is no need to have "goto out" here. > > I was considering two more examples of "goto out" in a branch right before the label: > - send_domain_memory_nonlive, > - send_domain_memory_live. > > Isn't it done this way to simplify the function extension: you won't need to add "goto out" to previous branch when adding new code? I'm not too fussed about this. Let's keep goto out. Acked-by: Wei Liu <wl@xen.org> I will apply this patch shortly. Wei.
Panyakin, Andrew writes ("Re: [XEN PATCH] libxc/migration: Abort migration on precopy policy request"):
> On 4/7/20 10:22 PM, Wei Liu wrote:
> > There is no need to have "goto out" here.
>
> I was considering two more examples of "goto out" in a branch right before the label:
> - send_domain_memory_nonlive,
> - send_domain_memory_live.
>
> Isn't it done this way to simplify the function extension: you won't need to add "goto out" to previous branch when adding new code?
Quite so.
Wei Liu writes ("Re: [XEN PATCH] libxc/migration: Abort migration on precopy policy request"):
> I'm not too fussed about this. Let's keep goto out.
Good :-).
> Acked-by: Wei Liu <wl@xen.org>
>
> I will apply this patch shortly.
Thanks,
Ian.
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c index fa736a311f..507274ce22 100644 --- a/tools/libxc/xc_sr_save.c +++ b/tools/libxc/xc_sr_save.c @@ -560,6 +560,12 @@ static int send_memory_live(struct xc_sr_context *ctx) } + if ( policy_decision == XGS_POLICY_ABORT ) { + PERROR("Abort precopy loop"); + rc = -1; + goto out; + } + out: xc_set_progress_prefix(xch, NULL); free(progress_str);
libxc defines XGS_POLICY_ABORT for precopy policy to signal that migration should be aborted (eg. if the estimated pause time is too huge for the instance). Default simple precopy policy never returns that, but it could be overriden with a custom one. Signed-off-by: Andrew Panyakin <apanyaki@amazon.com> --- tools/libxc/xc_sr_save.c | 6 ++++++ 1 file changed, 6 insertions(+)