Re: [PATCH v10 2/3] cpu-throttle: implement virtual CPU throttle

[Peter Xu <peterx@redhat.com>]

On Fri, Dec 31, 2021 at 12:36:40AM +0800, Hyman Huang wrote:
> > > +struct {
> > > +    DirtyLimitState *states;
> > > +    int max_cpus;
> > > +    unsigned long *bmap; /* running thread bitmap */
> > > +    unsigned long nr;
> > > +    QemuThread thread;
> > > +} *dirtylimit_state;
> > > +
> > > +static bool dirtylimit_quit = true;
> > 
> > Again, I think "quit" is not a good wording to show "whether dirtylimit is in
> > service".  How about "dirtylimit_global_enabled"?
> > 
> > You can actually use "dirtylimit_state" to show whether it's enabled already
> > (then drop the global value) since it's a pointer.  It shouldn't need to be
> > init-once-for-all, but we can alloc the strucuture wAhen dirty limit enabled
> > globally, and destroy it (and reset it to NULL) when globally disabled.
> > 
> > Then "whether it's enabled" is simply to check "!!dirtylimit_state" under BQL.
> Yes, checking pointer is fairly straightforword, but since dirtylimit thread
> also access the dirtylimit_state when doing the limit, if we free
> dirtylimit_state after last limited vcpu be canceled, dirtylimit thread
> would crash when reference null pointer. And this method turn out to
> introduce a mutex lock to protect dirtylimit_state, comparing with qatomic
> operation, which is better ?

I don't see much difference here on using either atomic or mutex, because it's
not a hot path.

If to use mutex and not overload BQL we can use a dirtylimit_mutex then before
referencing the pointer anywhere we need to fetch it, and release when sleep.

The only thing confusing to me about the global variable way is having
quit=true as initial value, and clearing it when start.  I think it'll work,
but just reads weird.

How about having "enabled" and "quit" as a normal threaded app?  Then:

  - When init: enabled=0 quit=0
  - When start: enabled=1 quit=0
  - When stop
    - main thread set enabled=1 quit=1
    - dirtylimit sees quit=1, goes to join()
    - main thread reset enable=quit=0

dirtylimit_in_service() should reference "enabled", and "quit" should be only
used for sync on exit.

Thanks,

-- 
Peter Xu