| Message ID | 668ef720-389f-4cf1-608e-64aca4f7c73d@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: more or less log-dirty related improvements | expand |
On 05/07/2021 16:13, Jan Beulich wrote: > In send_memory_live() the precise value the dirty_count struct field > gets initialized to doesn't matter much (apart from the triggering of > the log message in send_dirty_pages(), see below), but it is important > that it not be zero on the first iteration (or else send_dirty_pages() > won't get called at all). Saturate the initializer value at the maximum > value the field can hold. I've already explained why this is broken... > While there also initialize struct precopy_stats' respective field to a > more sane value: We don't really know how many dirty pages there are at > that point. > > In suspend_and_send_dirty() and verify_frames() the local variables > don't need initializing at all, as they're only an output from the > hypercall which gets invoked first thing. > > In send_checkpoint_dirty_pfn_list() the local variable can be dropped > altogether: It's optional to xc_logdirty_control() and not used anywhere > else. ... and why this is broken particularly in the context of a later change, and ... > > Note that in case the clipping actually takes effect, the "Bitmap > contained more entries than expected..." log message will trigger. This > being just an informational message, I don't think this is overly > concerning. ... why this isn't ok. Why do I bother wasting my time reviewing patches in the first place. ~Andrew
On 05.07.2021 17:41, Andrew Cooper wrote: > On 05/07/2021 16:13, Jan Beulich wrote: >> In send_memory_live() the precise value the dirty_count struct field >> gets initialized to doesn't matter much (apart from the triggering of >> the log message in send_dirty_pages(), see below), but it is important >> that it not be zero on the first iteration (or else send_dirty_pages() >> won't get called at all). Saturate the initializer value at the maximum >> value the field can hold. > > I've already explained why this is broken... > >> While there also initialize struct precopy_stats' respective field to a >> more sane value: We don't really know how many dirty pages there are at >> that point. >> >> In suspend_and_send_dirty() and verify_frames() the local variables >> don't need initializing at all, as they're only an output from the >> hypercall which gets invoked first thing. >> >> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >> altogether: It's optional to xc_logdirty_control() and not used anywhere >> else. > > ... and why this is broken particularly in the context of a later > change, and ... > >> >> Note that in case the clipping actually takes effect, the "Bitmap >> contained more entries than expected..." log message will trigger. This >> being just an informational message, I don't think this is overly >> concerning. > > ... why this isn't ok. > > Why do I bother wasting my time reviewing patches in the first place. I'm sorry Andrew, but no. I could as well reply "Why do I bother replying to your review comments?" You did say "I don't follow. Migration would be extremely broken if the first iteration didn't work correctly, so something else is going on here." which I replied to: "As per the title we're talking about overflow situation here. In particular the field did end up zero when ctx->save.p2m_size was 0x100000000. I'm not claiming in any way that the first iteration would generally not work." and then nothing else came back from you. I gave it a couple of days, taking silence as indication that my reply was satisfactory. Similarly e.g. for you saying "This is an external interface, and I'm not sure it will tolerate finding more than p2m_size allegedly dirty." with my reply "But you do realize that a few lines down from here there already was policy_stats->dirty_count = -1; ? Or are you trying to tell me that -1 (documented as indicating "unknown") is okay on subsequent iterations, but not on the first one? That's not being said anywhere ..." If you expect me to adjust patches rather then just replying verbally, you won't get around replying back when you get verbal feedback. Jan
Am Mon, 5 Jul 2021 17:13:28 +0200 schrieb Jan Beulich <jbeulich@suse.com>: > ctx->save.stats = (struct precopy_stats){ > - .dirty_count = ctx->save.p2m_size, > + .dirty_count = -1, > }; As said earlier, a consumer of these data may now be unable to initialize itself properly. Without the patch it would be able to size its private data structures properly to p2m_size. With the patch it can not know in advance what the upper limit might be. There is no in-tree consumer that is affected, and I do not have an out-of-tree consumer that might be broken by this change. Just saying.. Olaf
On 05.07.2021 19:26, Olaf Hering wrote: > Am Mon, 5 Jul 2021 17:13:28 +0200 > schrieb Jan Beulich <jbeulich@suse.com>: > >> ctx->save.stats = (struct precopy_stats){ >> - .dirty_count = ctx->save.p2m_size, >> + .dirty_count = -1, >> }; > > As said earlier, a consumer of these data may now be unable to initialize itself properly. Without the patch it would be able to size its private data structures properly to p2m_size. With the patch it can not know in advance what the upper limit might be. And as said before, besides the intention of this being usable for sizing purposes not being spelled out anywhere (instead it is explicitly documented that -1 means "unknown", and the value really _is_ unknown at this point; p2m_size is only a wrongly assumed upper bound), this is useless information as the size may change in the course of migration. It is a present limitation that this isn't handled properly, which I think would better not be baked into further places. > There is no in-tree consumer that is affected, and I do not have an out-of-tree consumer that might be broken by this change. > > Just saying.. Sure, but (as is the case for Andrew's replies) if you want me to change the patch, you need to not just "say" something, but provide convincing arguments. So far I've provided counter arguments which weren't proven wrong or at least inapplicable. Jan
Am Tue, 6 Jul 2021 08:39:21 +0200
schrieb Jan Beulich <jbeulich@suse.com>:
> the size may change in the course of migration
How can the p2m_size change? This upper limit is queried once, then all loops take this as loop counter.
Olaf
On 06.07.2021 08:46, Olaf Hering wrote: > Am Tue, 6 Jul 2021 08:39:21 +0200 > schrieb Jan Beulich <jbeulich@suse.com>: > >> the size may change in the course of migration > > How can the p2m_size change? This upper limit is queried once, then > all loops take this as loop counter. Well, that's part of the current limitation. The size would need re-querying before every iteration (or at the very least before the last one, after the domain has been paused). Jan
Am Tue, 6 Jul 2021 08:58:59 +0200 schrieb Jan Beulich <jbeulich@suse.com>: > Well, that's part of the current limitation. The size would need re-querying > before every iteration (or at the very least before the last one, after the > domain has been paused). Ah, you even wrote that. In addition each iteration should also query the state of the domU. Right now a shutdown is not handled and the dead domU is detected only at the very end. Olaf
On 05.07.21 17:13, Jan Beulich wrote: > In send_memory_live() the precise value the dirty_count struct field > gets initialized to doesn't matter much (apart from the triggering of > the log message in send_dirty_pages(), see below), but it is important > that it not be zero on the first iteration (or else send_dirty_pages() > won't get called at all). Saturate the initializer value at the maximum > value the field can hold. > > While there also initialize struct precopy_stats' respective field to a > more sane value: We don't really know how many dirty pages there are at > that point. > > In suspend_and_send_dirty() and verify_frames() the local variables > don't need initializing at all, as they're only an output from the > hypercall which gets invoked first thing. > > In send_checkpoint_dirty_pfn_list() the local variable can be dropped > altogether: It's optional to xc_logdirty_control() and not used anywhere > else. > > Note that in case the clipping actually takes effect, the "Bitmap > contained more entries than expected..." log message will trigger. This > being just an informational message, I don't think this is overly > concerning. Is there any real reason why the width of the stats fields can't be expanded to avoid clipping? This could avoid the need to set the initial value to -1, which seems one of the more controversial changes. Juergen
On 19.08.2021 12:20, Juergen Gross wrote: > On 05.07.21 17:13, Jan Beulich wrote: >> In send_memory_live() the precise value the dirty_count struct field >> gets initialized to doesn't matter much (apart from the triggering of >> the log message in send_dirty_pages(), see below), but it is important >> that it not be zero on the first iteration (or else send_dirty_pages() >> won't get called at all). Saturate the initializer value at the maximum >> value the field can hold. >> >> While there also initialize struct precopy_stats' respective field to a >> more sane value: We don't really know how many dirty pages there are at >> that point. >> >> In suspend_and_send_dirty() and verify_frames() the local variables >> don't need initializing at all, as they're only an output from the >> hypercall which gets invoked first thing. >> >> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >> altogether: It's optional to xc_logdirty_control() and not used anywhere >> else. >> >> Note that in case the clipping actually takes effect, the "Bitmap >> contained more entries than expected..." log message will trigger. This >> being just an informational message, I don't think this is overly >> concerning. > > Is there any real reason why the width of the stats fields can't be > expanded to avoid clipping? This could avoid the need to set the > initial value to -1, which seems one of the more controversial changes. While not impossible, it comes with a price tag, as we'd either need to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats or alter the underlying domctl. Neither of which looked either appealing or necessary to me; instead I'm still struggling with Andrew's comments, yet I didn't receive any clarification of further explanation. Plus I continue to think that statistics output like this shouldn't be assumed to be precise anyway, and for practical purposes I don't think it really matters how large the counts actually are once they've moved into the billions. Jan
On 19.08.21 13:06, Jan Beulich wrote: > On 19.08.2021 12:20, Juergen Gross wrote: >> On 05.07.21 17:13, Jan Beulich wrote: >>> In send_memory_live() the precise value the dirty_count struct field >>> gets initialized to doesn't matter much (apart from the triggering of >>> the log message in send_dirty_pages(), see below), but it is important >>> that it not be zero on the first iteration (or else send_dirty_pages() >>> won't get called at all). Saturate the initializer value at the maximum >>> value the field can hold. >>> >>> While there also initialize struct precopy_stats' respective field to a >>> more sane value: We don't really know how many dirty pages there are at >>> that point. >>> >>> In suspend_and_send_dirty() and verify_frames() the local variables >>> don't need initializing at all, as they're only an output from the >>> hypercall which gets invoked first thing. >>> >>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >>> altogether: It's optional to xc_logdirty_control() and not used anywhere >>> else. >>> >>> Note that in case the clipping actually takes effect, the "Bitmap >>> contained more entries than expected..." log message will trigger. This >>> being just an informational message, I don't think this is overly >>> concerning. >> >> Is there any real reason why the width of the stats fields can't be >> expanded to avoid clipping? This could avoid the need to set the >> initial value to -1, which seems one of the more controversial changes. > > While not impossible, it comes with a price tag, as we'd either need > to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats > or alter the underlying domctl. Neither of which looked either I was thinking about the domctl. Apart of struct xen_sysctl_page_offline_op this seems to be the only left domctl/sysctl structure limiting guest or host size to values being relevant. Changing those would be a sensible thing to do IMO. > appealing or necessary to me; instead I'm still struggling with > Andrew's comments, yet I didn't receive any clarification of further > explanation. Plus I continue to think that statistics output like this > shouldn't be assumed to be precise anyway, and for practical purposes > I don't think it really matters how large the counts actually are once > they've moved into the billions. In case the underlying problem can easily be avoided be changing the type of the field I don't see why this shouldn't be done. Especially as in this case a misleading message will just be avoided. Juergen
On 19.08.2021 13:25, Juergen Gross wrote: > On 19.08.21 13:06, Jan Beulich wrote: >> On 19.08.2021 12:20, Juergen Gross wrote: >>> On 05.07.21 17:13, Jan Beulich wrote: >>>> In send_memory_live() the precise value the dirty_count struct field >>>> gets initialized to doesn't matter much (apart from the triggering of >>>> the log message in send_dirty_pages(), see below), but it is important >>>> that it not be zero on the first iteration (or else send_dirty_pages() >>>> won't get called at all). Saturate the initializer value at the maximum >>>> value the field can hold. >>>> >>>> While there also initialize struct precopy_stats' respective field to a >>>> more sane value: We don't really know how many dirty pages there are at >>>> that point. >>>> >>>> In suspend_and_send_dirty() and verify_frames() the local variables >>>> don't need initializing at all, as they're only an output from the >>>> hypercall which gets invoked first thing. >>>> >>>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >>>> altogether: It's optional to xc_logdirty_control() and not used anywhere >>>> else. >>>> >>>> Note that in case the clipping actually takes effect, the "Bitmap >>>> contained more entries than expected..." log message will trigger. This >>>> being just an informational message, I don't think this is overly >>>> concerning. >>> >>> Is there any real reason why the width of the stats fields can't be >>> expanded to avoid clipping? This could avoid the need to set the >>> initial value to -1, which seems one of the more controversial changes. >> >> While not impossible, it comes with a price tag, as we'd either need >> to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats >> or alter the underlying domctl. Neither of which looked either > > I was thinking about the domctl. > > Apart of struct xen_sysctl_page_offline_op this seems to be the only > left domctl/sysctl structure limiting guest or host size to values > being relevant. Changing those would be a sensible thing to do IMO. Yet in the context of v1 of this series, which included "x86/paging: deal with log-dirty stats overflow" (now commit 17e91570c5a4) we settled on these fields not needing widening. This doesn't prevent us doing what you suggest, but it would look pretty odd to me at least. Jan
On 19.08.2021 13:51, Jan Beulich wrote: > On 19.08.2021 13:25, Juergen Gross wrote: >> On 19.08.21 13:06, Jan Beulich wrote: >>> On 19.08.2021 12:20, Juergen Gross wrote: >>>> On 05.07.21 17:13, Jan Beulich wrote: >>>>> In send_memory_live() the precise value the dirty_count struct field >>>>> gets initialized to doesn't matter much (apart from the triggering of >>>>> the log message in send_dirty_pages(), see below), but it is important >>>>> that it not be zero on the first iteration (or else send_dirty_pages() >>>>> won't get called at all). Saturate the initializer value at the maximum >>>>> value the field can hold. >>>>> >>>>> While there also initialize struct precopy_stats' respective field to a >>>>> more sane value: We don't really know how many dirty pages there are at >>>>> that point. >>>>> >>>>> In suspend_and_send_dirty() and verify_frames() the local variables >>>>> don't need initializing at all, as they're only an output from the >>>>> hypercall which gets invoked first thing. >>>>> >>>>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >>>>> altogether: It's optional to xc_logdirty_control() and not used anywhere >>>>> else. >>>>> >>>>> Note that in case the clipping actually takes effect, the "Bitmap >>>>> contained more entries than expected..." log message will trigger. This >>>>> being just an informational message, I don't think this is overly >>>>> concerning. >>>> >>>> Is there any real reason why the width of the stats fields can't be >>>> expanded to avoid clipping? This could avoid the need to set the >>>> initial value to -1, which seems one of the more controversial changes. >>> >>> While not impossible, it comes with a price tag, as we'd either need >>> to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats >>> or alter the underlying domctl. Neither of which looked either >> >> I was thinking about the domctl. >> >> Apart of struct xen_sysctl_page_offline_op this seems to be the only >> left domctl/sysctl structure limiting guest or host size to values >> being relevant. Changing those would be a sensible thing to do IMO. > > Yet in the context of v1 of this series, which included "x86/paging: > deal with log-dirty stats overflow" (now commit 17e91570c5a4) we > settled on these fields not needing widening. This doesn't prevent > us doing what you suggest, but it would look pretty odd to me at > least. And - just to make it very explicit - even if I went this route to address a controversial point, I'd still like to understand the reason for the original objection - if only for my own education. Jan
On 19.08.21 13:51, Jan Beulich wrote: > On 19.08.2021 13:25, Juergen Gross wrote: >> On 19.08.21 13:06, Jan Beulich wrote: >>> On 19.08.2021 12:20, Juergen Gross wrote: >>>> On 05.07.21 17:13, Jan Beulich wrote: >>>>> In send_memory_live() the precise value the dirty_count struct field >>>>> gets initialized to doesn't matter much (apart from the triggering of >>>>> the log message in send_dirty_pages(), see below), but it is important >>>>> that it not be zero on the first iteration (or else send_dirty_pages() >>>>> won't get called at all). Saturate the initializer value at the maximum >>>>> value the field can hold. >>>>> >>>>> While there also initialize struct precopy_stats' respective field to a >>>>> more sane value: We don't really know how many dirty pages there are at >>>>> that point. >>>>> >>>>> In suspend_and_send_dirty() and verify_frames() the local variables >>>>> don't need initializing at all, as they're only an output from the >>>>> hypercall which gets invoked first thing. >>>>> >>>>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped >>>>> altogether: It's optional to xc_logdirty_control() and not used anywhere >>>>> else. >>>>> >>>>> Note that in case the clipping actually takes effect, the "Bitmap >>>>> contained more entries than expected..." log message will trigger. This >>>>> being just an informational message, I don't think this is overly >>>>> concerning. >>>> >>>> Is there any real reason why the width of the stats fields can't be >>>> expanded to avoid clipping? This could avoid the need to set the >>>> initial value to -1, which seems one of the more controversial changes. >>> >>> While not impossible, it comes with a price tag, as we'd either need >>> to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats >>> or alter the underlying domctl. Neither of which looked either >> >> I was thinking about the domctl. >> >> Apart of struct xen_sysctl_page_offline_op this seems to be the only >> left domctl/sysctl structure limiting guest or host size to values >> being relevant. Changing those would be a sensible thing to do IMO. > > Yet in the context of v1 of this series, which included "x86/paging: > deal with log-dirty stats overflow" (now commit 17e91570c5a4) we > settled on these fields not needing widening. This doesn't prevent > us doing what you suggest, but it would look pretty odd to me at > least. Sorry I was too busy at that time to have a detailed look at the patches. TBH I'd rather undo the stats overflow handling and widen the fields. This is a rather simple patch and a much cleaner solution in the end. I'm not insisting on that, but in case I had to decide this would be my preferred way to handle it. Juergen
Jan Beulich writes ("Re: [PATCH v2 03/13] libxenguest: deal with log-dirty op stats overflow"):
> And - just to make it very explicit - even if I went this route to
> address a controversial point, I'd still like to understand the
> reason for the original objection - if only for my own education.
I agree with this position.
I have reread the discussions with this patch title in the Subject
line. I am still unclear on the precise nature of Andrew's objection.
All I could find was this from Andrew:
> ctx->save.stats = (struct precopy_stats){
> - .dirty_count = ctx->save.p2m_size,
> + .dirty_count = -1,
This is an external interface, and I'm not sure it will tolerate finding
more than p2m_size allegedly dirty.
and then the later comment by Andrew:
I've already explained why [various things]
Unfortunately that message did not contain any links to these previous
explanations. Andrew, where were they ? If we are not to go round
in circles, we need to write these things down and then refer to them
when relevant.
We had a conversation on irc today. I found today's IRC conversation
rather unsatisfactory. I gleaned from Andrew's comments that this he
was saying this callback is used by something in Citrix's product to
do with nVidia migration. Obviously we do not wish to break a
downstream. But this interface is badly documented and clear
explanations are lacking.
While I recognise Andrew's expertise in this area, I think that as
tools maintainer I need to go with the information I have.
The overflow issue is real and Jan has proposed a fix.
Andrew: do you have time to engage with Jan here on these API
questions ? If not, does anyone else have enough familiarity with
Citrix's use of this API to engage in a proper discussion about how
the overflow problem can be addressed ?
If effort is not available for this, that is of course fine. But as
an Open Source project we need to be able to move forwsrd with the
available input and contributions. So if no-one is able to explain
how this rather poor API is to be used, and help develop a solution to
the overflow problem, I should probably ack Jan's patch here.
Ian.
--- a/tools/libs/guest/xg_sr_restore.c +++ b/tools/libs/guest/xg_sr_restore.c @@ -452,7 +452,6 @@ static int send_checkpoint_dirty_pfn_lis unsigned int count, written; uint64_t i, *pfns = NULL; struct iovec *iov = NULL; - xc_shadow_op_stats_t stats = { 0, ctx->restore.p2m_size }; struct xc_sr_record rec = { .type = REC_TYPE_CHECKPOINT_DIRTY_PFN_LIST, }; @@ -462,7 +461,7 @@ static int send_checkpoint_dirty_pfn_lis if ( xc_logdirty_control( xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(dirty_bitmap), ctx->restore.p2m_size, - 0, &stats) != ctx->restore.p2m_size ) + 0, NULL) != ctx->restore.p2m_size ) { PERROR("Failed to retrieve logdirty bitmap"); goto err; --- a/tools/libs/guest/xg_sr_save.c +++ b/tools/libs/guest/xg_sr_save.c @@ -500,7 +500,9 @@ static int simple_precopy_policy(struct static int send_memory_live(struct xc_sr_context *ctx) { xc_interface *xch = ctx->xch; - xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size }; + xc_shadow_op_stats_t stats = { + .dirty_count = MIN(ctx->save.p2m_size, (typeof(stats.dirty_count))~0) + }; char *progress_str = NULL; unsigned int x = 0; int rc; @@ -519,7 +521,7 @@ static int send_memory_live(struct xc_sr goto out; ctx->save.stats = (struct precopy_stats){ - .dirty_count = ctx->save.p2m_size, + .dirty_count = -1, }; policy_stats = &ctx->save.stats; @@ -643,7 +645,7 @@ static int colo_merge_secondary_dirty_bi static int suspend_and_send_dirty(struct xc_sr_context *ctx) { xc_interface *xch = ctx->xch; - xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size }; + xc_shadow_op_stats_t stats; char *progress_str = NULL; int rc; DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, @@ -701,7 +703,7 @@ static int suspend_and_send_dirty(struct static int verify_frames(struct xc_sr_context *ctx) { xc_interface *xch = ctx->xch; - xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size }; + xc_shadow_op_stats_t stats; int rc; struct xc_sr_record rec = { .type = REC_TYPE_VERIFY };
In send_memory_live() the precise value the dirty_count struct field gets initialized to doesn't matter much (apart from the triggering of the log message in send_dirty_pages(), see below), but it is important that it not be zero on the first iteration (or else send_dirty_pages() won't get called at all). Saturate the initializer value at the maximum value the field can hold. While there also initialize struct precopy_stats' respective field to a more sane value: We don't really know how many dirty pages there are at that point. In suspend_and_send_dirty() and verify_frames() the local variables don't need initializing at all, as they're only an output from the hypercall which gets invoked first thing. In send_checkpoint_dirty_pfn_list() the local variable can be dropped altogether: It's optional to xc_logdirty_control() and not used anywhere else. Note that in case the clipping actually takes effect, the "Bitmap contained more entries than expected..." log message will trigger. This being just an informational message, I don't think this is overly concerning. Signed-off-by: Jan Beulich <jbeulich@suse.com>