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

[Hyman Huang]

在 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(黄勇)