Re: [PATCH v11 1/4] migration/dirtyrate: refactor dirty page rate calculation

[Hyman Huang <huangy81@chinatelecom.cn>]

在 2022/1/24 11:08, Peter Xu 写道:
> On Sat, Jan 22, 2022 at 11:22:37AM +0800, Hyman Huang wrote:
>>
>>
>> 在 2022/1/17 10:19, Peter Xu 写道:
>>> On Wed, Jan 05, 2022 at 01:14:06AM +0800, huangy81@chinatelecom.cn wrote:
>>>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>>>
>>>> +
>>>> +static void vcpu_dirty_stat_collect(VcpuStat *stat,
>>>> +                                    DirtyPageRecord *records,
>>>> +                                    bool start)
>>>> +{
>>>> +    CPUState *cpu;
>>>> +
>>>> +    CPU_FOREACH(cpu) {
>>>> +        if (!start && cpu->cpu_index >= stat->nvcpu) {
>>>> +            /*
>>>> +             * Never go there unless cpu is hot-plugged,
>>>> +             * just ignore in this case.
>>>> +             */
>>>> +            continue;
>>>> +        }
>>>
>>> As commented before, I think the only way to do it right is does not allow cpu
>>> plug/unplug during measurement..
>>>
>>> Say, even if index didn't get out of range, an unplug even should generate very
>>> stange output of the unplugged cpu.  Please see more below.
>>>
>>>> +        record_dirtypages(records, cpu, start);
>>>> +    }
>>>> +}
>>>> +
>>>> +int64_t vcpu_calculate_dirtyrate(int64_t calc_time_ms,
>>>> +                                 int64_t init_time_ms,
>>>> +                                 VcpuStat *stat,
>>>> +                                 unsigned int flag,
>>>> +                                 bool one_shot)
>>>> +{
>>>> +    DirtyPageRecord *records;
>>>> +    int64_t duration;
>>>> +    int64_t dirtyrate;
>>>> +    int i = 0;
>>>> +
>>>> +    cpu_list_lock();
>>>> +    records = vcpu_dirty_stat_alloc(stat);
>>>> +    vcpu_dirty_stat_collect(stat, records, true);
>>>> +    cpu_list_unlock();
>>>
>>> Continue with above - then I'm wondering whether we should just keep taking the
>>> lock until vcpu_dirty_stat_collect().
>>>
>>> Yes we could be taking the lock for a long time because of the sleep, but the
>>> main thread plug thread will just wait for it to complete and it is at least
>>> not a e.g. deadlock.
>>>
>>> The other solution is we do cpu_list_unlock() like this, but introduce another
>>> cpu_list_generation_id and boost it after any plug/unplug of cpu, aka, when cpu
>>> list changes.
>>>
>>> Then we record cpu generation ID at the entry of this function and retry the
>>> whole measurement if at some point we found generation ID changed (we need to
>>> fetch the gen ID after having the lock, of course).  That could avoid us taking
>>> the cpu list lock during dirty_stat_wait(), but it'll start to complicate cpu
>>> list locking rules.
>>>
>>> The simpler way is still just to take the lock, imho.
>>>
>> Hi, Peter, i'm working on this as you suggetion, and keep taking the
>> cpu_list_lock during dirty page rate calculation. I found the deadlock when
>> testing hotplug scenario, the logic is as the following:
>>
>> calc thread				qemu main thread
>> 1. take qemu_cpu_list_lock
>> 					1. take the BQL
>> 2. collect dirty page and wait		2. cpu hotplug
>> 					3. take qemu_cpu_list_lock
>> 3. take the BQL
>>
>> 4. sync dirty log			
>>
>> 5. release the BQL
>>
>> I just recall that is one of the reasons why i handle the plug/unplug
>> scenario(another is cpu plug may wait a little bit long time when dirtylimit
>> in service).
> 
> Ah I should have noticed the bql dependency with cpu list lock before..
> 
> I think having the cpu plug waiting for one sec is fine, because the mgmt app
> should be aware of both so it shouldn't even happen in practise (not good
> timing to plug during pre-migration).  However bql is definitely another
> story..  which I agree.
> 
>>
>> It seems that we have two strategies, one is just keep this logic untouched
>> in v12 and add "cpu_list_generation_id" implementaion in TODO list(once this
>> patchset been merged, i'll try that out)， another is introducing the
>> "cpu_list_generation_id" right now.
>>
>> What strategy do you prefer to?
> 
> I prefer having the gen_id patch.  The thing is it should be less than 10 lines
> and the logic should be fairly straightforward.  While if without it, it seems
> always on risk to use this new feature.
> 
> I hope I didn't overlook any existing mechanism to block cpu plug/unplug for
> some period, though, or we should use it.
> 
>>
>> Uh... I think the "unmatched_cnt" also kind of like this too, becauce once
>> we remove the "unmatched count" logic, the throttle algo is more likely to
>> oscillate and i prefer to add the "unmatched_cnt" in TODO list as above.
> 
> Could we tune the differential factor to make it less possible to oscillate?
> Uh... From certain angles, yes. When current dirty pate rate is nearly 
close to quota when dirtylimit in service, throttle achieve balance. 
Once the current dirty page rate arise a slight fluctuation(not much
oscillation), sleep time be adjusted which actually can be ignored.
> I still can't say I like "unmatched cnt" idea a lot..  From a PID pov (partial,
> integral, differential) you've already got partial + differential, and IMHO
> that "unmatched cnt" solution was trying to mimic an "integral" delta.  Instead
> of doing an mean value calculation (as in most integral system does) the
> "unmatched cnt" solution literally made it an array of 2 and it dropped the 1st
> element..  Hence a decision was made only from the 2nd data you collected.
> 
>  From that POV I think it's cleaner you add a real (but simple) integral algo
> into it?  It can be e.g. an array of 3, then when you do the math you use the
> average of the three dirty rates.  Would that work (and also look a bit
> cleaner)?
Yes, IMHO this is a more complete algo and we can try it out. So, let's 
see the v12 test result and decide whether above work should added to 
TODO list. :)
> 
> Thanks,
> 

-- 
Best regard

Hyman Huang(黄勇)