From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44189)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1YLVBh-0003yo-J4
	for qemu-devel@nongnu.org; Wed, 11 Feb 2015 06:10:39 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1YLVBb-0006Aa-Kb
	for qemu-devel@nongnu.org; Wed, 11 Feb 2015 06:10:33 -0500
Received: from mx1.redhat.com ([209.132.183.28]:58406)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1YLVBb-0006AV-E1
	for qemu-devel@nongnu.org; Wed, 11 Feb 2015 06:10:27 -0500
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <1423623986-590-3-git-send-email-liang.z.li@intel.com> (Liang
	Li's message of "Wed, 11 Feb 2015 11:06:16 +0800")
References: <1423623986-590-1-git-send-email-liang.z.li@intel.com>
	<1423623986-590-3-git-send-email-liang.z.li@intel.com>
Date: Wed, 11 Feb 2015 12:10:24 +0100
Message-ID: <87iof8bt5r.fsf@neno.neno>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [v5 02/12] migration: Add the framework of
	multi-thread compression
Reply-To: quintela@redhat.com
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Liang Li <liang.z.li@intel.com>
Cc: armbru@redhat.com, qemu-devel@nongnu.org, Yang Zhang <yang.z.zhang@intel.com>, amit.shah@redhat.com, lcapitulino@redhat.com, dgilbert@redhat.com

Liang Li <liang.z.li@intel.com> wrote:
> Add the code to create and destroy the multiple threads those will
> be used to do data compression. Left some functions empty to keep
> clearness, and the code will be added later.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> Reviewed-by: Dr.David Alan Gilbert <dgilbert@redhat.com>

And here I am again.

Reviewing patch 8, I found that we need to fix some things here.

> +static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
> +                                    ram_addr_t offset, bool last_stage)
> +{
> +    int bytes_sent = -1;
> +
> +    /* To be done*/
> +
> +    return bytes_sent;
> +}

We have three return values, here, that are not the same that for normal
pages

 0: this is the 1st page for a particular thread, nothing to sent yet
 n > 0: we are sending the previous compresed page for the choosen
        thread

Notice that the only way that ram_save_page() can return 0 is for xbzrle
when a page has modified but it has exactly the same value that before.

(it can have been modified twice, +1, -1 or whatever)

Notice that ram_save_page() can only return 0 (duplicate page) or > 0
(real size written)

> +
>  /*
>   * ram_find_and_save_block: Finds a page to send and sends it to f
>   *
> @@ -679,7 +751,12 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage)
>                  ram_bulk_stage = false;
>              }
>          } else {
> -            bytes_sent = ram_save_page(f, block, offset, last_stage);
> +            if (migrate_use_compression()) {
> +                bytes_sent = ram_save_compressed_page(f, block, offset,
> +                                                      last_stage);
> +            } else {
> +                bytes_sent = ram_save_page(f, block, offset, last_stage);
> +            }


I need more context, this is the corrent code

        } else {
            bytes_sent = ram_save_page(f, block, offset, last_stage);

            /* if page is unmodified, continue to the next */
            if (bytes_sent > 0) {
                last_sent_block = block;
                break;
            }
        }

And we should change to:

        } else if (migrate_use_compression()) {
            bytes_sent = ram_save_compressed_page(f, block, offset,
                                                  last_stage);
            last_sent_block = block;
            break;
        } else {
            bytes_sent = ram_save_page(f, block, offset, last_stage);

            /* if page is unmodified, continue to the next */
            if (bytes_sent > 0) {
                last_sent_block = block;
                break;
            }
        }

This would mean that we don't need to arrange for the zero byte return
on qemu.