[Qemu-devel] [PATCH 0/2] migration/postcopy: simplify postcopy_chunk_hostpages_pass

[Qemu-devel] [PATCH 0/2] migration/postcopy: simplify postcopy_chunk_hostpages_pass

[Wei Yang]

When looking into function postcopy_chunk_hostpages_pass(), we could use
alignment calculation to simplify it.

Wei Yang (2):
  migration/postcopy: simplify calculation of run_start and
    fixup_start_addr
  migration/postcopy: use QEMU_IS_ALIGNED to replace host_offset

 migration/ram.c | 37 +++++++------------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] migration/postcopy: simplify calculation of run_start and fixup_start_addr

[Wei Yang]

The purpose of the calculation is to find a HostPage which is partially
dirty.

  * fixup_start_addr points to the start of the HostPage to discard
  * run_start points to the next HostPage to check

While in the middle stage, there would two cases for run_start:

  * aligned with HostPage means this is not partially dirty
  * not aligned means this is partially dirty

When it is aligned, no work and calculation is necessary. run_start
already points to the start of next HostPage and is ready to continue.

When it is not aligned, the calculation could be simplified with:

  * fixup_start_addr = QEMU_ALIGN_DOWN(run_start, host_ratio)
  * run_start = QEMU_ALIGN_UP(run_start, host_ratio)

By doing so, run_start always points to the next HostPage to check.
fixup_start_addr always points to the HostPage to discard.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index c9585487ac..d86661a015 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2956,7 +2956,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
     }
 
     while (run_start < pages) {
-        unsigned long fixup_start_addr;
         unsigned long host_offset;
 
         /*
@@ -2964,45 +2963,26 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
          * page, then we need to fixup this host page.
          */
         host_offset = run_start % host_ratio;
-        if (host_offset) {
-            fixup_start_addr = run_start - host_offset;
-            /*
-             * This host page has gone, the next loop iteration starts
-             * from after the fixup
-             */
-            run_start = fixup_start_addr + host_ratio;
-        } else {
+        if (!host_offset) {
             /* Find the end of this run */
-            unsigned long run_end;
             if (unsent_pass) {
-                run_end = find_next_bit(unsentmap, pages, run_start + 1);
+                run_start = find_next_bit(unsentmap, pages, run_start + 1);
             } else {
-                run_end = find_next_zero_bit(bitmap, pages, run_start + 1);
+                run_start = find_next_zero_bit(bitmap, pages, run_start + 1);
             }
             /*
              * If the end isn't at the start of a host page, then the
              * run doesn't finish at the end of a host page
              * and we need to discard.
              */
-            host_offset = run_end % host_ratio;
-            if (host_offset) {
-                fixup_start_addr = run_end - host_offset;
-                /*
-                 * This host page has gone, the next loop iteration starts
-                 * from after the fixup
-                 */
-                run_start = fixup_start_addr + host_ratio;
-            } else {
-                /*
-                 * No discards on this iteration, next loop starts from
-                 * next sent/dirty page
-                 */
-                run_start = run_end + 1;
-            }
+            host_offset = run_start % host_ratio;
         }
 
         if (host_offset) {
             unsigned long page;
+            unsigned long fixup_start_addr = QEMU_ALIGN_DOWN(run_start,
+                                                             host_ratio);
+            run_start = QEMU_ALIGN_UP(run_start, host_ratio);
 
             /* Tell the destination to discard this page */
             if (unsent_pass || !test_bit(fixup_start_addr, unsentmap)) {
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] migration/postcopy: use QEMU_IS_ALIGNED to replace host_offset

[Wei Yang]

Use QEMU_IS_ALIGNED for the check, it would be more consistent with
other align calculations.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d86661a015..dfbf71c580 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2956,14 +2956,12 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
     }
 
     while (run_start < pages) {
-        unsigned long host_offset;
 
         /*
          * If the start of this run of pages is in the middle of a host
          * page, then we need to fixup this host page.
          */
-        host_offset = run_start % host_ratio;
-        if (!host_offset) {
+        if (QEMU_IS_ALIGNED(run_start, host_ratio)) {
             /* Find the end of this run */
             if (unsent_pass) {
                 run_start = find_next_bit(unsentmap, pages, run_start + 1);
@@ -2975,10 +2973,9 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
              * run doesn't finish at the end of a host page
              * and we need to discard.
              */
-            host_offset = run_start % host_ratio;
         }
 
-        if (host_offset) {
+        if (!QEMU_IS_ALIGNED(run_start, host_ratio)) {
             unsigned long page;
             unsigned long fixup_start_addr = QEMU_ALIGN_DOWN(run_start,
                                                              host_ratio);
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] migration/postcopy: simplify calculation of run_start and fixup_start_addr

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> The purpose of the calculation is to find a HostPage which is partially
> dirty.
> 
>   * fixup_start_addr points to the start of the HostPage to discard
>   * run_start points to the next HostPage to check
> 
> While in the middle stage, there would two cases for run_start:
> 
>   * aligned with HostPage means this is not partially dirty
>   * not aligned means this is partially dirty
> 
> When it is aligned, no work and calculation is necessary. run_start
> already points to the start of next HostPage and is ready to continue.
> 
> When it is not aligned, the calculation could be simplified with:
> 
>   * fixup_start_addr = QEMU_ALIGN_DOWN(run_start, host_ratio)
>   * run_start = QEMU_ALIGN_UP(run_start, host_ratio)
> 
> By doing so, run_start always points to the next HostPage to check.
> fixup_start_addr always points to the HostPage to discard.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Yes that's a lot shorter;  note it doess it on top of your cleanup
from a couple of weeks back.

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


> ---
>  migration/ram.c | 34 +++++++---------------------------
>  1 file changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c9585487ac..d86661a015 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2956,7 +2956,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>      }
>  
>      while (run_start < pages) {
> -        unsigned long fixup_start_addr;
>          unsigned long host_offset;
>  
>          /*
> @@ -2964,45 +2963,26 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>           * page, then we need to fixup this host page.
>           */
>          host_offset = run_start % host_ratio;
> -        if (host_offset) {
> -            fixup_start_addr = run_start - host_offset;
> -            /*
> -             * This host page has gone, the next loop iteration starts
> -             * from after the fixup
> -             */
> -            run_start = fixup_start_addr + host_ratio;
> -        } else {
> +        if (!host_offset) {
>              /* Find the end of this run */
> -            unsigned long run_end;
>              if (unsent_pass) {
> -                run_end = find_next_bit(unsentmap, pages, run_start + 1);
> +                run_start = find_next_bit(unsentmap, pages, run_start + 1);
>              } else {
> -                run_end = find_next_zero_bit(bitmap, pages, run_start + 1);
> +                run_start = find_next_zero_bit(bitmap, pages, run_start + 1);
>              }
>              /*
>               * If the end isn't at the start of a host page, then the
>               * run doesn't finish at the end of a host page
>               * and we need to discard.
>               */
> -            host_offset = run_end % host_ratio;
> -            if (host_offset) {
> -                fixup_start_addr = run_end - host_offset;
> -                /*
> -                 * This host page has gone, the next loop iteration starts
> -                 * from after the fixup
> -                 */
> -                run_start = fixup_start_addr + host_ratio;
> -            } else {
> -                /*
> -                 * No discards on this iteration, next loop starts from
> -                 * next sent/dirty page
> -                 */
> -                run_start = run_end + 1;
> -            }
> +            host_offset = run_start % host_ratio;
>          }
>  
>          if (host_offset) {
>              unsigned long page;
> +            unsigned long fixup_start_addr = QEMU_ALIGN_DOWN(run_start,
> +                                                             host_ratio);
> +            run_start = QEMU_ALIGN_UP(run_start, host_ratio);
>  
>              /* Tell the destination to discard this page */
>              if (unsent_pass || !test_bit(fixup_start_addr, unsentmap)) {
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] migration/postcopy: use QEMU_IS_ALIGNED to replace host_offset

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Use QEMU_IS_ALIGNED for the check, it would be more consistent with
> other align calculations.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

> ---
>  migration/ram.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index d86661a015..dfbf71c580 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2956,14 +2956,12 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>      }
>  
>      while (run_start < pages) {
> -        unsigned long host_offset;
>  
>          /*
>           * If the start of this run of pages is in the middle of a host
>           * page, then we need to fixup this host page.
>           */
> -        host_offset = run_start % host_ratio;
> -        if (!host_offset) {
> +        if (QEMU_IS_ALIGNED(run_start, host_ratio)) {
>              /* Find the end of this run */
>              if (unsent_pass) {
>                  run_start = find_next_bit(unsentmap, pages, run_start + 1);
> @@ -2975,10 +2973,9 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>               * run doesn't finish at the end of a host page
>               * and we need to discard.
>               */
> -            host_offset = run_start % host_ratio;
>          }
>  
> -        if (host_offset) {
> +        if (!QEMU_IS_ALIGNED(run_start, host_ratio)) {
>              unsigned long page;
>              unsigned long fixup_start_addr = QEMU_ALIGN_DOWN(run_start,
>                                                               host_ratio);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] migration/postcopy: simplify calculation of run_start and fixup_start_addr

[Wei Yang]

On Tue, Aug 06, 2019 at 03:51:17PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> The purpose of the calculation is to find a HostPage which is partially
>> dirty.
>> 
>>   * fixup_start_addr points to the start of the HostPage to discard
>>   * run_start points to the next HostPage to check
>> 
>> While in the middle stage, there would two cases for run_start:
>> 
>>   * aligned with HostPage means this is not partially dirty
>>   * not aligned means this is partially dirty
>> 
>> When it is aligned, no work and calculation is necessary. run_start
>> already points to the start of next HostPage and is ready to continue.
>> 
>> When it is not aligned, the calculation could be simplified with:
>> 
>>   * fixup_start_addr = QEMU_ALIGN_DOWN(run_start, host_ratio)
>>   * run_start = QEMU_ALIGN_UP(run_start, host_ratio)
>> 
>> By doing so, run_start always points to the next HostPage to check.
>> fixup_start_addr always points to the HostPage to discard.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Yes that's a lot shorter;  note it doess it on top of your cleanup
>from a couple of weeks back.
>

You are right, I forget to mention it :-)

>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>
>> ---
>>  migration/ram.c | 34 +++++++---------------------------
>>  1 file changed, 7 insertions(+), 27 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index c9585487ac..d86661a015 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2956,7 +2956,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>>      }
>>  
>>      while (run_start < pages) {
>> -        unsigned long fixup_start_addr;
>>          unsigned long host_offset;
>>  
>>          /*
>> @@ -2964,45 +2963,26 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
>>           * page, then we need to fixup this host page.
>>           */
>>          host_offset = run_start % host_ratio;
>> -        if (host_offset) {
>> -            fixup_start_addr = run_start - host_offset;
>> -            /*
>> -             * This host page has gone, the next loop iteration starts
>> -             * from after the fixup
>> -             */
>> -            run_start = fixup_start_addr + host_ratio;
>> -        } else {
>> +        if (!host_offset) {
>>              /* Find the end of this run */
>> -            unsigned long run_end;
>>              if (unsent_pass) {
>> -                run_end = find_next_bit(unsentmap, pages, run_start + 1);
>> +                run_start = find_next_bit(unsentmap, pages, run_start + 1);
>>              } else {
>> -                run_end = find_next_zero_bit(bitmap, pages, run_start + 1);
>> +                run_start = find_next_zero_bit(bitmap, pages, run_start + 1);
>>              }
>>              /*
>>               * If the end isn't at the start of a host page, then the
>>               * run doesn't finish at the end of a host page
>>               * and we need to discard.
>>               */
>> -            host_offset = run_end % host_ratio;
>> -            if (host_offset) {
>> -                fixup_start_addr = run_end - host_offset;
>> -                /*
>> -                 * This host page has gone, the next loop iteration starts
>> -                 * from after the fixup
>> -                 */
>> -                run_start = fixup_start_addr + host_ratio;
>> -            } else {
>> -                /*
>> -                 * No discards on this iteration, next loop starts from
>> -                 * next sent/dirty page
>> -                 */
>> -                run_start = run_end + 1;
>> -            }
>> +            host_offset = run_start % host_ratio;
>>          }
>>  
>>          if (host_offset) {
>>              unsigned long page;
>> +            unsigned long fixup_start_addr = QEMU_ALIGN_DOWN(run_start,
>> +                                                             host_ratio);
>> +            run_start = QEMU_ALIGN_UP(run_start, host_ratio);
>>  
>>              /* Tell the destination to discard this page */
>>              if (unsent_pass || !test_bit(fixup_start_addr, unsentmap)) {
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 0/2] migration/postcopy: simplify postcopy_chunk_hostpages_pass

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> When looking into function postcopy_chunk_hostpages_pass(), we could use
> alignment calculation to simplify it.
> 
> Wei Yang (2):
>   migration/postcopy: simplify calculation of run_start and
>     fixup_start_addr
>   migration/postcopy: use QEMU_IS_ALIGNED to replace host_offset
> 
>  migration/ram.c | 37 +++++++------------------------------
>  1 file changed, 7 insertions(+), 30 deletions(-)

Queued

> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK