| Message ID | 1b24d2b5-14ff-6e4f-6fa5-d91d75e315a4@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: more or less log-dirty related improvements | expand |
On 26/04/2022 11:24, Jan Beulich wrote: > In send_memory_live() the precise value the dirty_count struct field > gets initialized to doesn't matter much Yes it does. And as you keep on refusing to actually fix the bugs pointed out during review, this entire series is NACKED, seeing as you've also repeatedly failed to address feedback from previous revisions on other patches. ~Andrew
On 26.04.2022 12:54, Andrew Cooper wrote: > On 26/04/2022 11:24, Jan Beulich wrote: >> In send_memory_live() the precise value the dirty_count struct field >> gets initialized to doesn't matter much > > Yes it does. > > And as you keep on refusing to actually fix the bugs pointed out during > review, this entire series is NACKED, seeing as you've also repeatedly > failed to address feedback from previous revisions on other patches. Excuse me? Afair I did respond back, requesting further input from you. It is this input which hasn't come forward; I don't see how I could have made changes when I don't know what exactly I need to do. This is emphasized by Ian giving a provisional ack on 'libxenguest: short- circuit "all-dirty" handling', depending on you not proving him, Jürgen (who had reviewed what is now the first two patches), and me wrong. There again was no response from you. In such a case no response can only mean "what I've heard back was satisfactory". Also may I remind you that you're not in the position to NACK this entire series. You can latch '"open" objections', yes, but it is at best unfair to record objections without then working towards resolving them, by simply remaining silent towards the request for further input. This looks to be a repeating pattern, which is rather frustrating. I fully recognize that in many areas you know things better than I do, but this doesn't mean you're always right and I'm always wrong. Therefore, like e.g. for patch 1 in this series, you will want to actively convince the three people who think the change is okay, rather than simply saying "no" with apparently none of the three understanding why you say so. I also view it as at least questionable behavior to NACK an entire series when there are clearly issues which want taking care of. If not by the patches in their present shape, then by whatever is suitable (working out of which is why discussion needs to continue until clarity is actually reached). Jan
--- a/tools/libs/guest/xg_sr_restore.c +++ b/tools/libs/guest/xg_sr_restore.c @@ -427,7 +427,6 @@ static int send_checkpoint_dirty_pfn_lis unsigned long i; uint64_t *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, }; @@ -437,7 +436,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 @@ -495,7 +495,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; @@ -511,7 +513,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; @@ -633,7 +635,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, @@ -694,7 +696,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> --- TBD: To address Andrew's objection, Jürgen suggests to widen struct xen_domctl_shadow_op_stats'es fields; yet within this same series we had settled on there not being a need (17e91570c5a4).