On Fri, Jan 14, 2022 at 09:30:39AM +0800, Hyman Huang wrote:
>
>
> 在 2022/1/14 0:22, Markus Armbruster 写道:
> > Peter Xu <pet...@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
