diff mbox series

[v2,03/13] libxenguest: deal with log-dirty op stats overflow

Message ID 668ef720-389f-4cf1-608e-64aca4f7c73d@suse.com (mailing list archive)
State New
Headers show
Series x86: more or less log-dirty related improvements | expand

Commit Message

Jan Beulich July 5, 2021, 3:13 p.m. UTC
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>

Comments

Andrew Cooper July 5, 2021, 3:41 p.m. UTC | #1
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
Jan Beulich July 5, 2021, 3:53 p.m. UTC | #2
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
Olaf Hering July 5, 2021, 5:26 p.m. UTC | #3
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
Jan Beulich July 6, 2021, 6:39 a.m. UTC | #4
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
Olaf Hering July 6, 2021, 6:46 a.m. UTC | #5
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
Jan Beulich July 6, 2021, 6:58 a.m. UTC | #6
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
Olaf Hering July 6, 2021, 7:15 a.m. UTC | #7
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
Juergen Gross Aug. 19, 2021, 10:20 a.m. UTC | #8
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
Jan Beulich Aug. 19, 2021, 11:06 a.m. UTC | #9
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
Juergen Gross Aug. 19, 2021, 11:25 a.m. UTC | #10
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
Jan Beulich Aug. 19, 2021, 11:51 a.m. UTC | #11
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
Jan Beulich Aug. 19, 2021, 11:53 a.m. UTC | #12
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
Juergen Gross Aug. 19, 2021, 2:29 p.m. UTC | #13
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
Ian Jackson Sept. 2, 2021, 4:57 p.m. UTC | #14
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.
diff mbox series

Patch

--- 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 };