Peter Xu <pet...@redhat.com> wrote:
> On Tue, Mar 28, 2017 at 08:43:37PM +0200, Juan Quintela wrote:
>> Peter Xu <pet...@redhat.com> wrote:
>> > On Thu, Mar 23, 2017 at 09:45:04PM +0100, Juan Quintela wrote:
>> >> Once there rename it to its actual meaning, zero_pages.
>> >>
>> >> Signed-off-by: Juan Quintela <quint...@redhat.com>
>> >> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>> >
>> > Reviewed-by: Peter Xu <pet...@redhat.com>
>> >
>> > Will post a question below though (not directly related to this patch
>> > but context-wide)...
>> >> {
>> >> int pages = -1;
>> >>
>> >> if (is_zero_range(p, TARGET_PAGE_SIZE)) {
>> >> - acct_info.dup_pages++;
>> >> + rs->zero_pages++;
>> >> *bytes_transferred += save_page_header(f, block,
>> >> offset |
>> >> RAM_SAVE_FLAG_COMPRESS);
>> >> qemu_put_byte(f, 0);
>> >> @@ -822,11 +826,11 @@ static int ram_save_page(RAMState *rs,
>> >> MigrationState *ms, QEMUFile *f,
>> >> if (bytes_xmit > 0) {
>> >> acct_info.norm_pages++;
>> >> } else if (bytes_xmit == 0) {
>> >> - acct_info.dup_pages++;
>> >> + rs->zero_pages++;
>> >
>> > This code path looks suspicous... since iiuc currently it should only
>> > be triggered by RDMA case, and I believe here qemu_rdma_save_page()
>> > should have met something wrong (so that it didn't return with
>> > RAM_SAVE_CONTROL_DELAYED). Then is it correct we do increase zero page
>> > counting unconditionally here? (hmm, the default bytes_xmit is zero as
>> > well...)
>>
>> My head hurts at this point.
>
> Sorry about that! :(
Hahaha, it was a ""figure of speak" O:-)
>> ok. bytse_xmit can only be zero if we called qemu_rdma_save_page() with
>> size=0 or there has been an RDMA error. We ver call the function with
>> size = 0. And if there is one error, we are in very bady shape already.
>>
>> > Another thing is that I see when RDMA is enabled we are updating
>> > accounting info with acct_update_position(), while we updated it here
>> > as well. Is this an issue of duplicated accounting?
>>
>> I think stats and rdma are not right. I have to check more that.
>
> Sorry to have led the discussion too far away from the topic. I guess
> it'll be perfectly okay to just mark this as TODO item, and we can
> just move on with current series (and I believe you have further
> patches after this big one :).
Yeap.
> Out of curiosity - to what extent are people using migration with
> RDMA? Should that be "very rare"? Thanks,
I don't really have numbers. Some customers find it very important, but
I don't have a good idea of how to put it.
Later, Juan.
