Re: [PATCH v3] migration/throttle: Add cpu-throttle-tailslow migration parameter

[zhukeqian <zhukeqian1@huawei.com>]

Hi Dr.David,

Sorry for the reply delay, just come back from holiday.

On 2020/4/30 22:12, Dr. David Alan Gilbert wrote:
> * Keqian Zhu (zhukeqian1@huawei.com) wrote:
>> At the tail stage of throttling, the Guest is very sensitive to
>> CPU percentage while the @cpu-throttle-increment is excessive
>> usually at tail stage.
[...]
>> -static void mig_throttle_guest_down(void)
>> +static void mig_throttle_guest_down(uint64_t bytes_dirty_period,
>> +                                    uint64_t bytes_dirty_threshold)
>>  {
>>      MigrationState *s = migrate_get_current();
>>      uint64_t pct_initial = s->parameters.cpu_throttle_initial;
>> -    uint64_t pct_icrement = s->parameters.cpu_throttle_increment;
>> +    uint64_t pct_increment = s->parameters.cpu_throttle_increment;
>> +    bool pct_tailslow = s->parameters.cpu_throttle_tailslow;
>>      int pct_max = s->parameters.max_cpu_throttle;
>>  
>> +    uint64_t throttle_now = cpu_throttle_get_percentage();
>> +    uint64_t cpu_now, cpu_ideal, throttle_inc;
>> +
>>      /* We have not started throttling yet. Let's start it. */
>>      if (!cpu_throttle_active()) {
>>          cpu_throttle_set(pct_initial);
>>      } else {
>>          /* Throttling already on, just increase the rate */
>> -        cpu_throttle_set(MIN(cpu_throttle_get_percentage() + pct_icrement,
>> -                         pct_max));
>> +        if (!pct_tailslow) {
>> +            throttle_inc = pct_increment;
>> +        } else {
>> +            /* Compute the ideal CPU percentage used by Guest, which may
>> +             * make the dirty rate match the dirty rate threshold. */
>> +            cpu_now = 100 - throttle_now;
>> +            cpu_ideal = cpu_now * (bytes_dirty_threshold * 1.0 /
>> +                        bytes_dirty_period);
> 
> I worry if we need a divide-by-0 check; but that seems unlikely.
mig_throttle_guest_down is called when bytes_dirty_period is bigger than
bytes_dirty_threshold, and bytes_dirty_threshold is of unsigned type, so
bytes_dirty_period will not be zero here. I will add an assert check here
to make it clear.

> Now if that worked out as huge, then I think the MIN's guard it even
> with overflow below, so I think we're OK.
Yes, it will not exceed legacy increment.

> 
>> +            throttle_inc = MIN(cpu_now - cpu_ideal, pct_increment);
>> +        }
>> +        cpu_throttle_set(MIN(throttle_now + throttle_inc, pct_max));
>>      }
>>  }
>>  
[...]
>> -- 
>> 2.19.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
Thanks,
Keqian
> 
> .
>