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

[Peter Xu]

On Fri, Jan 14, 2022 at 09:30:39AM +0800, Hyman Huang wrote:
> 
> 
> 在 2022/1/14 0:22, Markus Armbruster 写道:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > 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.
> > 
> > Quick interjection without having bothered to understand the details:
> > correct use of atomics and memory barriers is *much* harder than correct
> > use of locks.  Stick to locks unless you *know* they impair performance

Yong,

Just a heads up - You seem to have replied something but there's really nothing
I saw... it happened multiple times, so I hope you didn't miss it by sending
something empty.

I agree with Markus, and that's also what I wanted to express too (it's not a
perf critical path, so we don't necessarily need to use atomics; obviously I
failed again on using English to express myself.. :).  But I don't urge it if
the atomics works pretty simple and well.  I think I'll read the atomic version
you posted first and I'll comment again there.

Thanks,

-- 
Peter Xu