[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 2/3] cpu-throttle: implement virtual CPU throttle
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v10 2/3] cpu-throttle: implement virtual CPU throttle |
Date: |
Thu, 13 Jan 2022 17:22:40 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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.
[...]