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. [...]