| Message ID | 20210723193444.133412-8-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Sanity checks memory transaction when releasing BQL | expand |
On 23.07.21 21:34, Peter Xu wrote: > The prepare function before unlocking BQL. There're only three places that can > release the BQL: unlock(), cond_wait() or cond_timedwait(). > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > softmmu/cpus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/softmmu/cpus.c b/softmmu/cpus.c > index 9131f77f87..6085f8edbe 100644 > --- a/softmmu/cpus.c > +++ b/softmmu/cpus.c > @@ -66,6 +66,10 @@ > > static QemuMutex qemu_global_mutex; > > +static void qemu_mutex_unlock_iothread_prepare(void) > +{ > +} > + > bool cpu_is_stopped(CPUState *cpu) > { > return cpu->stopped || !runstate_is_running(); > @@ -523,16 +527,19 @@ void qemu_mutex_unlock_iothread(void) > { > g_assert(qemu_mutex_iothread_locked()); > iothread_locked = false; > + qemu_mutex_unlock_iothread_prepare(); > qemu_mutex_unlock(&qemu_global_mutex); > } > > void qemu_cond_wait_iothread(QemuCond *cond) > { > + qemu_mutex_unlock_iothread_prepare(); > qemu_cond_wait(cond, &qemu_global_mutex); > } > > void qemu_cond_timedwait_iothread(QemuCond *cond, int ms) > { > + qemu_mutex_unlock_iothread_prepare(); > qemu_cond_timedwait(cond, &qemu_global_mutex, ms); > } > > I'd squash this patch into the next one. I don't quite like the function name, but don't really have a better suggestion .... maybe qemu_mutex_might_unlock_iothread(), similar to might_sleep() or might_fault() in the kernel. (although here it's pretty clear and not a "might"; could be useful in other context where we might conditionally unlock the BQL at some point in the future, though)
On Tue, Jul 27, 2021 at 02:59:26PM +0200, David Hildenbrand wrote: > On 23.07.21 21:34, Peter Xu wrote: > > The prepare function before unlocking BQL. There're only three places that can > > release the BQL: unlock(), cond_wait() or cond_timedwait(). > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > softmmu/cpus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/softmmu/cpus.c b/softmmu/cpus.c > > index 9131f77f87..6085f8edbe 100644 > > --- a/softmmu/cpus.c > > +++ b/softmmu/cpus.c > > @@ -66,6 +66,10 @@ > > static QemuMutex qemu_global_mutex; > > +static void qemu_mutex_unlock_iothread_prepare(void) > > +{ > > +} > > + > > bool cpu_is_stopped(CPUState *cpu) > > { > > return cpu->stopped || !runstate_is_running(); > > @@ -523,16 +527,19 @@ void qemu_mutex_unlock_iothread(void) > > { > > g_assert(qemu_mutex_iothread_locked()); > > iothread_locked = false; > > + qemu_mutex_unlock_iothread_prepare(); > > qemu_mutex_unlock(&qemu_global_mutex); > > } > > void qemu_cond_wait_iothread(QemuCond *cond) > > { > > + qemu_mutex_unlock_iothread_prepare(); > > qemu_cond_wait(cond, &qemu_global_mutex); > > } > > void qemu_cond_timedwait_iothread(QemuCond *cond, int ms) > > { > > + qemu_mutex_unlock_iothread_prepare(); > > qemu_cond_timedwait(cond, &qemu_global_mutex, ms); > > } > > > > I'd squash this patch into the next one. > > I don't quite like the function name, but don't really have a better > suggestion .... maybe qemu_mutex_might_unlock_iothread(), similar to > might_sleep() or might_fault() in the kernel. (although here it's pretty > clear and not a "might"; could be useful in other context where we might > conditionally unlock the BQL at some point in the future, though) Yes, IMHO "might" describes a capability of doing something, here it's not (this one should only be called right before releasing bql, not within any context of having some capability). The other option I thought was "pre" but it will be just a short version of "prepare". Let me know if you have a better suggestion on naming. :) Otherwise I'll keep the naming, squash this patch into the next and keep your r-b for that. Thanks,
On 27.07.21 18:08, Peter Xu wrote: > On Tue, Jul 27, 2021 at 02:59:26PM +0200, David Hildenbrand wrote: >> On 23.07.21 21:34, Peter Xu wrote: >>> The prepare function before unlocking BQL. There're only three places that can >>> release the BQL: unlock(), cond_wait() or cond_timedwait(). >>> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> softmmu/cpus.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c >>> index 9131f77f87..6085f8edbe 100644 >>> --- a/softmmu/cpus.c >>> +++ b/softmmu/cpus.c >>> @@ -66,6 +66,10 @@ >>> static QemuMutex qemu_global_mutex; >>> +static void qemu_mutex_unlock_iothread_prepare(void) >>> +{ >>> +} >>> + >>> bool cpu_is_stopped(CPUState *cpu) >>> { >>> return cpu->stopped || !runstate_is_running(); >>> @@ -523,16 +527,19 @@ void qemu_mutex_unlock_iothread(void) >>> { >>> g_assert(qemu_mutex_iothread_locked()); >>> iothread_locked = false; >>> + qemu_mutex_unlock_iothread_prepare(); >>> qemu_mutex_unlock(&qemu_global_mutex); >>> } >>> void qemu_cond_wait_iothread(QemuCond *cond) >>> { >>> + qemu_mutex_unlock_iothread_prepare(); >>> qemu_cond_wait(cond, &qemu_global_mutex); >>> } >>> void qemu_cond_timedwait_iothread(QemuCond *cond, int ms) >>> { >>> + qemu_mutex_unlock_iothread_prepare(); >>> qemu_cond_timedwait(cond, &qemu_global_mutex, ms); >>> } >>> >> >> I'd squash this patch into the next one. >> >> I don't quite like the function name, but don't really have a better >> suggestion .... maybe qemu_mutex_might_unlock_iothread(), similar to >> might_sleep() or might_fault() in the kernel. (although here it's pretty >> clear and not a "might"; could be useful in other context where we might >> conditionally unlock the BQL at some point in the future, though) > > Yes, IMHO "might" describes a capability of doing something, here it's not > (this one should only be called right before releasing bql, not within any > context of having some capability). The other option I thought was "pre" but > it will be just a short version of "prepare". > > Let me know if you have a better suggestion on naming. :) Otherwise I'll keep > the naming, squash this patch into the next and keep your r-b for that. Fine with me :)
diff --git a/softmmu/cpus.c b/softmmu/cpus.c index 9131f77f87..6085f8edbe 100644 --- a/softmmu/cpus.c +++ b/softmmu/cpus.c @@ -66,6 +66,10 @@ static QemuMutex qemu_global_mutex; +static void qemu_mutex_unlock_iothread_prepare(void) +{ +} + bool cpu_is_stopped(CPUState *cpu) { return cpu->stopped || !runstate_is_running(); @@ -523,16 +527,19 @@ void qemu_mutex_unlock_iothread(void) { g_assert(qemu_mutex_iothread_locked()); iothread_locked = false; + qemu_mutex_unlock_iothread_prepare(); qemu_mutex_unlock(&qemu_global_mutex); } void qemu_cond_wait_iothread(QemuCond *cond) { + qemu_mutex_unlock_iothread_prepare(); qemu_cond_wait(cond, &qemu_global_mutex); } void qemu_cond_timedwait_iothread(QemuCond *cond, int ms) { + qemu_mutex_unlock_iothread_prepare(); qemu_cond_timedwait(cond, &qemu_global_mutex, ms); }
The prepare function before unlocking BQL. There're only three places that can release the BQL: unlock(), cond_wait() or cond_timedwait(). Signed-off-by: Peter Xu <peterx@redhat.com> --- softmmu/cpus.c | 7 +++++++ 1 file changed, 7 insertions(+)