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 (address@hidden) 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 <address@hidden>
>
>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 <address@hidden>
>
>
>> ---
>>  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 / address@hidden / Manchester, UK

-- 
Wei Yang
Help you, Help me