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
