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

[Markus Armbruster <armbru@redhat.com>]

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.

[...]