Re: [PATCH v5 1/4] migration: do not flush_compressed_data at the end of each iteration

[Juan Quintela <quintela@redhat.com>]

guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>
> flush_compressed_data() needs to wait all compression threads to
> finish their work, after that all threads are free until the
> migration feeds new request to them, reducing its call can improve
> the throughput and use CPU resource more effectively
> We do not need to flush all threads at the end of iteration, the
> data can be kept locally until the memory block is changed or
> memory migration starts over in that case we will meet a dirtied
> page which may still exists in compression threads's ring
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>

I am not so sure about this patch.

Right now, we warantee that after each iteration, all data is written
before we start a new round.

This patch changes to only "flush" the compression threads if we have
"synched" with the kvm migration bitmap.  Idea is good but as far as I
can see:

- we already call flush_compressed_data() inside firnd_dirty_block if we
  synchronize the bitmap.  So, at least, we need to update
  dirty_sync_count there.

- queued pages are "interesting", but I am not sure if compression and
  postcopy work well together.

So, if we don't need to call flush_compressed_data() every round, then
the one inside find_dirty_block() should be enough.  Otherwise, I can't
see why we need this other.


Independent of this patch:
- We always send data for every compression thread without testing if
there is any there.


> @@ -3212,7 +3225,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          }
>          i++;
>      }
> -    flush_compressed_data(rs);
>      rcu_read_unlock();
>  
>      /*

Why is not enough just to remove this call to flush_compressed_data?

Later, Juan.