[v6,10/13] migration: Respect postcopy request order in preemption mode

With preemption mode on, when we see a postcopy request that was requesting
for exactly the page that we have preempted before (so we've partially sent
the page already via PRECOPY channel and it got preempted by another
postcopy request), currently we drop the request so that after all the
other postcopy requests are serviced then we'll go back to precopy stream
and start to handle that.

We dropped the request because we can't send it via postcopy channel since
the precopy channel already contains partial of the data, and we can only
send a huge page via one channel as a whole.  We can't split a huge page
into two channels.

That's a very corner case and that works, but there's a change on the order
of postcopy requests that we handle since we're postponing this (unlucky)
postcopy request to be later than the other queued postcopy requests.  The
problem is there's a possibility that when the guest was very busy, the
postcopy queue can be always non-empty, it means this dropped request will
never be handled until the end of postcopy migration. So, there's a chance
that there's one dest QEMU vcpu thread waiting for a page fault for an
extremely long time just because it's unluckily accessing the specific page
that was preempted before.

The worst case time it needs can be as long as the whole postcopy migration
procedure.  It's extremely unlikely to happen, but when it happens it's not
good.

The root cause of this problem is because we treat pss->postcopy_requested
variable as with two meanings bound together, as the variable shows:

  1. Whether this page request is urgent, and,
  2. Which channel we should use for this page request.

With the old code, when we set postcopy_requested it means either both (1)
and (2) are true, or both (1) and (2) are false.  We can never have (1)
and (2) to have different values.

However it doesn't necessarily need to be like that.  It's very legal that
there's one request that has (1) very high urgency, but (2) we'd like to
use the precopy channel.  Just like the corner case we were discussing
above.

To differenciate the two meanings better, introduce a new field called
postcopy_target_channel, showing which channel we should use for this page
request, so as to cover the old meaning (2) only.  Then we leave the
postcopy_requested variable to stand only for meaning (1), which is the
urgency of this page request.

With this change, we can easily boost priority of a preempted precopy page
as long as we know that page is also requested as a postcopy page.  So with
the new approach in get_queued_page() instead of dropping that request, we
send it right away with the precopy channel so we get back the ordering of
the page faults just like how they're requested on dest.

Alongside, I touched up find_dirty_block() to only set the postcopy fields
in the pss section if we're going through a postcopy migration.  That's a
very light optimization and shouldn't affect much.

Reported-by: manish.mishra@nutanix.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 69 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 14 deletions(-)

* Peter Xu (peterx@redhat.com) wrote:
> With preemption mode on, when we see a postcopy request that was requesting
> for exactly the page that we have preempted before (so we've partially sent
> the page already via PRECOPY channel and it got preempted by another
> postcopy request), currently we drop the request so that after all the
> other postcopy requests are serviced then we'll go back to precopy stream
> and start to handle that.
> 
> We dropped the request because we can't send it via postcopy channel since
> the precopy channel already contains partial of the data, and we can only
> send a huge page via one channel as a whole.  We can't split a huge page
> into two channels.
> 
> That's a very corner case and that works, but there's a change on the order
> of postcopy requests that we handle since we're postponing this (unlucky)
> postcopy request to be later than the other queued postcopy requests.  The
> problem is there's a possibility that when the guest was very busy, the
> postcopy queue can be always non-empty, it means this dropped request will
> never be handled until the end of postcopy migration. So, there's a chance
> that there's one dest QEMU vcpu thread waiting for a page fault for an
> extremely long time just because it's unluckily accessing the specific page
> that was preempted before.
> 
> The worst case time it needs can be as long as the whole postcopy migration
> procedure.  It's extremely unlikely to happen, but when it happens it's not
> good.
> 
> The root cause of this problem is because we treat pss->postcopy_requested
> variable as with two meanings bound together, as the variable shows:
> 
>   1. Whether this page request is urgent, and,
>   2. Which channel we should use for this page request.
> 
> With the old code, when we set postcopy_requested it means either both (1)
> and (2) are true, or both (1) and (2) are false.  We can never have (1)
> and (2) to have different values.
> 
> However it doesn't necessarily need to be like that.  It's very legal that
> there's one request that has (1) very high urgency, but (2) we'd like to
> use the precopy channel.  Just like the corner case we were discussing
> above.
> 
> To differenciate the two meanings better, introduce a new field called
> postcopy_target_channel, showing which channel we should use for this page
> request, so as to cover the old meaning (2) only.  Then we leave the
> postcopy_requested variable to stand only for meaning (1), which is the
> urgency of this page request.
> 
> With this change, we can easily boost priority of a preempted precopy page
> as long as we know that page is also requested as a postcopy page.  So with
> the new approach in get_queued_page() instead of dropping that request, we
> send it right away with the precopy channel so we get back the ordering of
> the page faults just like how they're requested on dest.
> 
> Alongside, I touched up find_dirty_block() to only set the postcopy fields
> in the pss section if we're going through a postcopy migration.  That's a
> very light optimization and shouldn't affect much.
> 
> Reported-by: manish.mishra@nutanix.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

So I think this is OK; getting a bit complicated!

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> ---
>  migration/ram.c | 69 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 9d76db8491..fdcd9984fa 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -441,8 +441,28 @@ struct PageSearchStatus {
>      unsigned long page;
>      /* Set once we wrap around */
>      bool         complete_round;
> -    /* Whether current page is explicitly requested by postcopy */
> +    /*
> +     * [POSTCOPY-ONLY] Whether current page is explicitly requested by
> +     * postcopy.  When set, the request is "urgent" because the dest QEMU
> +     * threads are waiting for us.
> +     */
>      bool         postcopy_requested;
> +    /*
> +     * [POSTCOPY-ONLY] The target channel to use to send current page.
> +     *
> +     * Note: This may _not_ match with the value in postcopy_requested
> +     * above. Let's imagine the case where the postcopy request is exactly
> +     * the page that we're sending in progress during precopy. In this case
> +     * we'll have postcopy_requested set to true but the target channel
> +     * will be the precopy channel (so that we don't split brain on that
> +     * specific page since the precopy channel already contains partial of
> +     * that page data).
> +     *
> +     * Besides that specific use case, postcopy_target_channel should
> +     * always be equal to postcopy_requested, because by default we send
> +     * postcopy pages via postcopy preempt channel.
> +     */
> +    bool         postcopy_target_channel;
>  };
>  typedef struct PageSearchStatus PageSearchStatus;
>  
> @@ -496,6 +516,9 @@ static QemuCond decomp_done_cond;
>  static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>                                   ram_addr_t offset, uint8_t *source_buf);
>  
> +static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss,
> +                                     bool postcopy_requested);
> +
>  static void *do_data_compress(void *opaque)
>  {
>      CompressParam *param = opaque;
> @@ -1516,8 +1539,14 @@ retry:
>   */
>  static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
>  {
> -    /* This is not a postcopy requested page */
> -    pss->postcopy_requested = false;
> +    if (migration_in_postcopy()) {
> +        /*
> +         * This is not a postcopy requested page, mark it "not urgent", and
> +         * use precopy channel to send it.
> +         */
> +        pss->postcopy_requested = false;
> +        pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
> +    }

Do you need the 'if' here?

Dave

>      pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
>      if (pss->complete_round && pss->block == rs->last_seen_block &&
> @@ -2038,15 +2067,20 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
>      RAMBlock  *block;
>      ram_addr_t offset;
>  
> -again:
>      block = unqueue_page(rs, &offset);
>  
>      if (block) {
>          /* See comment above postcopy_preempted_contains() */
>          if (postcopy_preempted_contains(rs, block, offset)) {
>              trace_postcopy_preempt_hit(block->idstr, offset);
> -            /* This request is dropped */
> -            goto again;
> +            /*
> +             * If what we preempted previously was exactly what we're
> +             * requesting right now, restore the preempted precopy
> +             * immediately, boosting its priority as it's requested by
> +             * postcopy.
> +             */
> +            postcopy_preempt_restore(rs, pss, true);
> +            return true;
>          }
>      } else {
>          /*
> @@ -2070,7 +2104,9 @@ again:
>           * really rare.
>           */
>          pss->complete_round = false;
> +        /* Mark it an urgent request, meanwhile using POSTCOPY channel */
>          pss->postcopy_requested = true;
> +        pss->postcopy_target_channel = RAM_CHANNEL_POSTCOPY;
>      }
>  
>      return !!block;
> @@ -2324,7 +2360,8 @@ static bool postcopy_preempt_triggered(RAMState *rs)
>      return rs->postcopy_preempt_state.preempted;
>  }
>  
> -static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
> +static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss,
> +                                     bool postcopy_requested)
>  {
>      PostcopyPreemptState *state = &rs->postcopy_preempt_state;
>  
> @@ -2332,8 +2369,15 @@ static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
>  
>      pss->block = state->ram_block;
>      pss->page = state->ram_page;
> -    /* This is not a postcopy request but restoring previous precopy */
> -    pss->postcopy_requested = false;
> +
> +    /* Whether this is a postcopy request? */
> +    pss->postcopy_requested = postcopy_requested;
> +    /*
> +     * When restoring a preempted page, the old data resides in PRECOPY
> +     * slow channel, even if postcopy_requested is set.  So always use
> +     * PRECOPY channel here.
> +     */
> +    pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
>  
>      trace_postcopy_preempt_restored(pss->block->idstr, pss->page);
>  
> @@ -2344,12 +2388,9 @@ static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
>  static void postcopy_preempt_choose_channel(RAMState *rs, PageSearchStatus *pss)
>  {
>      MigrationState *s = migrate_get_current();
> -    unsigned int channel;
> +    unsigned int channel = pss->postcopy_target_channel;
>      QEMUFile *next;
>  
> -    channel = pss->postcopy_requested ?
> -        RAM_CHANNEL_POSTCOPY : RAM_CHANNEL_PRECOPY;
> -
>      if (channel != rs->postcopy_channel) {
>          if (channel == RAM_CHANNEL_PRECOPY) {
>              next = s->to_dst_file;
> @@ -2505,7 +2546,7 @@ static int ram_find_and_save_block(RAMState *rs)
>               * preempted precopy.  Otherwise find the next dirty bit.
>               */
>              if (postcopy_preempt_triggered(rs)) {
> -                postcopy_preempt_restore(rs, &pss);
> +                postcopy_preempt_restore(rs, &pss, false);
>                  found = true;
>              } else {
>                  /* priority queue empty, so just search for something dirty */
> -- 
> 2.32.0
>

On 23/05/22 4:26 pm, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
>> With preemption mode on, when we see a postcopy request that was requesting
>> for exactly the page that we have preempted before (so we've partially sent
>> the page already via PRECOPY channel and it got preempted by another
>> postcopy request), currently we drop the request so that after all the
>> other postcopy requests are serviced then we'll go back to precopy stream
>> and start to handle that.
>>
>> We dropped the request because we can't send it via postcopy channel since
>> the precopy channel already contains partial of the data, and we can only
>> send a huge page via one channel as a whole.  We can't split a huge page
>> into two channels.
>>
>> That's a very corner case and that works, but there's a change on the order
>> of postcopy requests that we handle since we're postponing this (unlucky)
>> postcopy request to be later than the other queued postcopy requests.  The
>> problem is there's a possibility that when the guest was very busy, the
>> postcopy queue can be always non-empty, it means this dropped request will
>> never be handled until the end of postcopy migration. So, there's a chance
>> that there's one dest QEMU vcpu thread waiting for a page fault for an
>> extremely long time just because it's unluckily accessing the specific page
>> that was preempted before.
>>
>> The worst case time it needs can be as long as the whole postcopy migration
>> procedure.  It's extremely unlikely to happen, but when it happens it's not
>> good.
>>
>> The root cause of this problem is because we treat pss->postcopy_requested
>> variable as with two meanings bound together, as the variable shows:
>>
>>    1. Whether this page request is urgent, and,
>>    2. Which channel we should use for this page request.
>>
>> With the old code, when we set postcopy_requested it means either both (1)
>> and (2) are true, or both (1) and (2) are false.  We can never have (1)
>> and (2) to have different values.
>>
>> However it doesn't necessarily need to be like that.  It's very legal that
>> there's one request that has (1) very high urgency, but (2) we'd like to
>> use the precopy channel.  Just like the corner case we were discussing
>> above.
>>
>> To differenciate the two meanings better, introduce a new field called
>> postcopy_target_channel, showing which channel we should use for this page
>> request, so as to cover the old meaning (2) only.  Then we leave the
>> postcopy_requested variable to stand only for meaning (1), which is the
>> urgency of this page request.
>>
>> With this change, we can easily boost priority of a preempted precopy page
>> as long as we know that page is also requested as a postcopy page.  So with
>> the new approach in get_queued_page() instead of dropping that request, we
>> send it right away with the precopy channel so we get back the ordering of
>> the page faults just like how they're requested on dest.
>>
>> Alongside, I touched up find_dirty_block() to only set the postcopy fields
>> in the pss section if we're going through a postcopy migration.  That's a
>> very light optimization and shouldn't affect much.
>>
>> Reported-by: manish.mishra@nutanix.com
>> Signed-off-by: Peter Xu <peterx@redhat.com>
> So I think this is OK; getting a bit complicated!
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Manish Mishra <manish.mishra@nutanix.com>
>
>> ---
>>   migration/ram.c | 69 +++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 55 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 9d76db8491..fdcd9984fa 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -441,8 +441,28 @@ struct PageSearchStatus {
>>       unsigned long page;
>>       /* Set once we wrap around */
>>       bool         complete_round;
>> -    /* Whether current page is explicitly requested by postcopy */
>> +    /*
>> +     * [POSTCOPY-ONLY] Whether current page is explicitly requested by
>> +     * postcopy.  When set, the request is "urgent" because the dest QEMU
>> +     * threads are waiting for us.
>> +     */
>>       bool         postcopy_requested;
>> +    /*
>> +     * [POSTCOPY-ONLY] The target channel to use to send current page.
>> +     *
>> +     * Note: This may _not_ match with the value in postcopy_requested
>> +     * above. Let's imagine the case where the postcopy request is exactly
>> +     * the page that we're sending in progress during precopy. In this case
>> +     * we'll have postcopy_requested set to true but the target channel
>> +     * will be the precopy channel (so that we don't split brain on that
>> +     * specific page since the precopy channel already contains partial of
>> +     * that page data).
>> +     *
>> +     * Besides that specific use case, postcopy_target_channel should
>> +     * always be equal to postcopy_requested, because by default we send
>> +     * postcopy pages via postcopy preempt channel.
>> +     */
>> +    bool         postcopy_target_channel;
>>   };
>>   typedef struct PageSearchStatus PageSearchStatus;
>>   
>> @@ -496,6 +516,9 @@ static QemuCond decomp_done_cond;
>>   static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>>                                    ram_addr_t offset, uint8_t *source_buf);
>>   
>> +static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss,
>> +                                     bool postcopy_requested);
>> +
>>   static void *do_data_compress(void *opaque)
>>   {
>>       CompressParam *param = opaque;
>> @@ -1516,8 +1539,14 @@ retry:
>>    */
>>   static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
>>   {
>> -    /* This is not a postcopy requested page */
>> -    pss->postcopy_requested = false;
>> +    if (migration_in_postcopy()) {
>> +        /*
>> +         * This is not a postcopy requested page, mark it "not urgent", and
>> +         * use precopy channel to send it.
>> +         */
>> +        pss->postcopy_requested = false;
>> +        pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
>> +    }
> Do you need the 'if' here?
>
> Dave
>
>>       pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
>>       if (pss->complete_round && pss->block == rs->last_seen_block &&
>> @@ -2038,15 +2067,20 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
>>       RAMBlock  *block;
>>       ram_addr_t offset;
>>   
>> -again:
>>       block = unqueue_page(rs, &offset);
>>   
>>       if (block) {
>>           /* See comment above postcopy_preempted_contains() */
>>           if (postcopy_preempted_contains(rs, block, offset)) {
>>               trace_postcopy_preempt_hit(block->idstr, offset);
>> -            /* This request is dropped */
>> -            goto again;
>> +            /*
>> +             * If what we preempted previously was exactly what we're
>> +             * requesting right now, restore the preempted precopy
>> +             * immediately, boosting its priority as it's requested by
>> +             * postcopy.
>> +             */
>> +            x(rs, pss, true);
>> +            return true;
>>           }
>>       } else {
>>           /*
>> @@ -2070,7 +2104,9 @@ again:
>>            * really rare.
>>            */
>>           pss->complete_round = false;
>> +        /* Mark it an urgent request, meanwhile using POSTCOPY channel */
>>           pss->postcopy_requested = true;
>> +        pss->postcopy_target_channel = RAM_CHANNEL_POSTCOPY;
>>       }
>>   
>>       return !!block;
>> @@ -2324,7 +2360,8 @@ static bool postcopy_preempt_triggered(RAMState *rs)
>>       return rs->postcopy_preempt_state.preempted;
>>   }
>>   
>> -static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
>> +static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss,
>> +                                     bool postcopy_requested)
>>   {
>>       PostcopyPreemptState *state = &rs->postcopy_preempt_state;
>>   
>> @@ -2332,8 +2369,15 @@ static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
>>   
>>       pss->block = state->ram_block;
>>       pss->page = state->ram_page;
>> -    /* This is not a postcopy request but restoring previous precopy */
>> -    pss->postcopy_requested = false;
>> +
>> +    /* Whether this is a postcopy request? */
>> +    pss->postcopy_requested = postcopy_requested;
>> +    /*
>> +     * When restoring a preempted page, the old data resides in PRECOPY
>> +     * slow channel, even if postcopy_requested is set.  So always use
>> +     * PRECOPY channel here.
>> +     */
>> +    pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
>>   
>>       trace_postcopy_preempt_restored(pss->block->idstr, pss->page);
>>   
>> @@ -2344,12 +2388,9 @@ static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss)
>>   static void postcopy_preempt_choose_channel(RAMState *rs, PageSearchStatus *pss)
>>   {
>>       MigrationState *s = migrate_get_current();
>> -    unsigned int channel;
>> +    unsigned int channel = pss->postcopy_target_channel;
>>       QEMUFile *next;
>>   
>> -    channel = pss->postcopy_requested ?
>> -        RAM_CHANNEL_POSTCOPY : RAM_CHANNEL_PRECOPY;
>> -
>>       if (channel != rs->postcopy_channel) {
>>           if (channel == RAM_CHANNEL_PRECOPY) {
>>               next = s->to_dst_file;
>> @@ -2505,7 +2546,7 @@ static int ram_find_and_save_block(RAMState *rs)
>>                * preempted precopy.  Otherwise find the next dirty bit.
>>                */
>>               if (postcopy_preempt_triggered(rs)) {
>> -                postcopy_preempt_restore(rs, &pss);
>> +                postcopy_preempt_restore(rs, &pss, false);
>>                   found = true;
>>               } else {
>>                   /* priority queue empty, so just search for something dirty */
>> -- 
>> 2.32.0
>>

On Mon, May 23, 2022 at 11:56:14AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > With preemption mode on, when we see a postcopy request that was requesting
> > for exactly the page that we have preempted before (so we've partially sent
> > the page already via PRECOPY channel and it got preempted by another
> > postcopy request), currently we drop the request so that after all the
> > other postcopy requests are serviced then we'll go back to precopy stream
> > and start to handle that.
> > 
> > We dropped the request because we can't send it via postcopy channel since
> > the precopy channel already contains partial of the data, and we can only
> > send a huge page via one channel as a whole.  We can't split a huge page
> > into two channels.
> > 
> > That's a very corner case and that works, but there's a change on the order
> > of postcopy requests that we handle since we're postponing this (unlucky)
> > postcopy request to be later than the other queued postcopy requests.  The
> > problem is there's a possibility that when the guest was very busy, the
> > postcopy queue can be always non-empty, it means this dropped request will
> > never be handled until the end of postcopy migration. So, there's a chance
> > that there's one dest QEMU vcpu thread waiting for a page fault for an
> > extremely long time just because it's unluckily accessing the specific page
> > that was preempted before.
> > 
> > The worst case time it needs can be as long as the whole postcopy migration
> > procedure.  It's extremely unlikely to happen, but when it happens it's not
> > good.
> > 
> > The root cause of this problem is because we treat pss->postcopy_requested
> > variable as with two meanings bound together, as the variable shows:
> > 
> >   1. Whether this page request is urgent, and,
> >   2. Which channel we should use for this page request.
> > 
> > With the old code, when we set postcopy_requested it means either both (1)
> > and (2) are true, or both (1) and (2) are false.  We can never have (1)
> > and (2) to have different values.
> > 
> > However it doesn't necessarily need to be like that.  It's very legal that
> > there's one request that has (1) very high urgency, but (2) we'd like to
> > use the precopy channel.  Just like the corner case we were discussing
> > above.
> > 
> > To differenciate the two meanings better, introduce a new field called
> > postcopy_target_channel, showing which channel we should use for this page
> > request, so as to cover the old meaning (2) only.  Then we leave the
> > postcopy_requested variable to stand only for meaning (1), which is the
> > urgency of this page request.
> > 
> > With this change, we can easily boost priority of a preempted precopy page
> > as long as we know that page is also requested as a postcopy page.  So with
> > the new approach in get_queued_page() instead of dropping that request, we
> > send it right away with the precopy channel so we get back the ordering of
> > the page faults just like how they're requested on dest.
> > 
> > Alongside, I touched up find_dirty_block() to only set the postcopy fields
> > in the pss section if we're going through a postcopy migration.  That's a
> > very light optimization and shouldn't affect much.
> > 
> > Reported-by: manish.mishra@nutanix.com
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> So I think this is OK; getting a bit complicated!

Yes it is.  I added some more comment, hopefully it'll help a little bit.

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!

> >  static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
> >  {
> > -    /* This is not a postcopy requested page */
> > -    pss->postcopy_requested = false;
> > +    if (migration_in_postcopy()) {
> > +        /*
> > +         * This is not a postcopy requested page, mark it "not urgent", and
> > +         * use precopy channel to send it.
> > +         */
> > +        pss->postcopy_requested = false;
> > +        pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY;
> > +    }
> 
> Do you need the 'if' here?

Hmm good question..  precopy should always have these two fields cleared
anyway so I wanted to avoid setting them every time, but I just noticed
that pss is not initialized at all when used..

static int ram_find_and_save_block(RAMState *rs)
{
    PageSearchStatus pss;
    ...
}

So either we'd reset pss explicitly on these fields, or simpler - let me
drop the if..

Thanks,