Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
---
job.c | 46 +++++++++++++++++-----------------------------
1 file changed, 17 insertions(+), 29 deletions(-)
diff --git a/job.c b/job.c
index 8fecf38960..89ceb53434 100644
--- a/job.c
+++ b/job.c
@@ -79,16 +79,6 @@ struct JobTxn {
* job_enter. */
static QemuMutex job_mutex;
-static void job_lock(void)
-{
- qemu_mutex_lock(&job_mutex);
-}
-
-static void job_unlock(void)
-{
- qemu_mutex_unlock(&job_mutex);
-}
-
static void __attribute__((__constructor__)) job_init(void)
{
qemu_mutex_init(&job_mutex);
@@ -437,21 +427,19 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job))
return;
}
- job_lock();
- if (job->busy) {
- job_unlock();
- return;
- }
+ WITH_QEMU_LOCK_GUARD(&job_mutex) {
+ if (job->busy) {
+ return;
+ }
- if (fn && !fn(job)) {
- job_unlock();
- return;
- }
+ if (fn && !fn(job)) {
+ return;
+ }
- assert(!job->deferred_to_main_loop);
- timer_del(&job->sleep_timer);
- job->busy = true;
- job_unlock();
+ assert(!job->deferred_to_main_loop);
+ timer_del(&job->sleep_timer);
+ job->busy = true;
+ }
aio_co_enter(job->aio_context, job->co);
}
@@ -468,13 +456,13 @@ void job_enter(Job *job)
* called explicitly. */
static void coroutine_fn job_do_yield(Job *job, uint64_t ns)
{
- job_lock();
- if (ns != -1) {
- timer_mod(&job->sleep_timer, ns);
+ WITH_QEMU_LOCK_GUARD(&job_mutex) {
+ if (ns != -1) {
+ timer_mod(&job->sleep_timer, ns);
+ }
+ job->busy = false;
+ job_event_idle(job);
}
- job->busy = false;
- job_event_idle(job);
- job_unlock();
qemu_coroutine_yield();
/* Set by job_enter_cond() before re-entering the coroutine. */
--
2.25.1
On 9/29/20 9:42 AM, Elena Afanasova wrote: > Signed-off-by: Elena Afanasova <eafanasova@gmail.com> Hi, can I have a commit message here, please? > --- > job.c | 46 +++++++++++++++++----------------------------- > 1 file changed, 17 insertions(+), 29 deletions(-) > > diff --git a/job.c b/job.c > index 8fecf38960..89ceb53434 100644 > --- a/job.c > +++ b/job.c > @@ -79,16 +79,6 @@ struct JobTxn { > * job_enter. */ > static QemuMutex job_mutex; > > -static void job_lock(void) > -{ > - qemu_mutex_lock(&job_mutex); > -} > - > -static void job_unlock(void) > -{ > - qemu_mutex_unlock(&job_mutex); > -} > - > static void __attribute__((__constructor__)) job_init(void) > { > qemu_mutex_init(&job_mutex); > @@ -437,21 +427,19 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job)) > return; > } > > - job_lock(); > - if (job->busy) { > - job_unlock(); > - return; > - } > + WITH_QEMU_LOCK_GUARD(&job_mutex) { > + if (job->busy) { > + return; > + } > > - if (fn && !fn(job)) { > - job_unlock(); > - return; > - } > + if (fn && !fn(job)) { > + return; > + } > > - assert(!job->deferred_to_main_loop); > - timer_del(&job->sleep_timer); > - job->busy = true; > - job_unlock(); > + assert(!job->deferred_to_main_loop); > + timer_del(&job->sleep_timer); > + job->busy = true; > + } > aio_co_enter(job->aio_context, job->co); > } > > @@ -468,13 +456,13 @@ void job_enter(Job *job) > * called explicitly. */ > static void coroutine_fn job_do_yield(Job *job, uint64_t ns) > { > - job_lock(); > - if (ns != -1) { > - timer_mod(&job->sleep_timer, ns); > + WITH_QEMU_LOCK_GUARD(&job_mutex) { > + if (ns != -1) { > + timer_mod(&job->sleep_timer, ns); > + } > + job->busy = false; > + job_event_idle(job); Is this new macro safe to use in a coroutine context? > } > - job->busy = false; > - job_event_idle(job); > - job_unlock(); > qemu_coroutine_yield(); > > /* Set by job_enter_cond() before re-entering the coroutine. */ > I haven't looked into WITH_QEMU_LOCK_GUARD before, I assume it's new. If it works like I think it does, this change seems good. (I'm assuming it works like a Python context manager and it drops the lock when it leaves the scope of the macro using GCC/Clang language extensions.)
On Tue, 2020-09-29 at 14:04 -0400, John Snow wrote: > On 9/29/20 9:42 AM, Elena Afanasova wrote: > > Signed-off-by: Elena Afanasova <eafanasova@gmail.com> > > Hi, can I have a commit message here, please? > > > --- > > job.c | 46 +++++++++++++++++----------------------------- > > 1 file changed, 17 insertions(+), 29 deletions(-) > > > > diff --git a/job.c b/job.c > > index 8fecf38960..89ceb53434 100644 > > --- a/job.c > > +++ b/job.c > > @@ -79,16 +79,6 @@ struct JobTxn { > > * job_enter. */ > > static QemuMutex job_mutex; > > > > -static void job_lock(void) > > -{ > > - qemu_mutex_lock(&job_mutex); > > -} > > - > > -static void job_unlock(void) > > -{ > > - qemu_mutex_unlock(&job_mutex); > > -} > > - > > static void __attribute__((__constructor__)) job_init(void) > > { > > qemu_mutex_init(&job_mutex); > > @@ -437,21 +427,19 @@ void job_enter_cond(Job *job, bool(*fn)(Job > > *job)) > > return; > > } > > > > - job_lock(); > > - if (job->busy) { > > - job_unlock(); > > - return; > > - } > > + WITH_QEMU_LOCK_GUARD(&job_mutex) { > > + if (job->busy) { > > + return; > > + } > > > > - if (fn && !fn(job)) { > > - job_unlock(); > > - return; > > - } > > + if (fn && !fn(job)) { > > + return; > > + } > > > > - assert(!job->deferred_to_main_loop); > > - timer_del(&job->sleep_timer); > > - job->busy = true; > > - job_unlock(); > > + assert(!job->deferred_to_main_loop); > > + timer_del(&job->sleep_timer); > > + job->busy = true; > > + } > > aio_co_enter(job->aio_context, job->co); > > } > > > > @@ -468,13 +456,13 @@ void job_enter(Job *job) > > * called explicitly. */ > > static void coroutine_fn job_do_yield(Job *job, uint64_t ns) > > { > > - job_lock(); > > - if (ns != -1) { > > - timer_mod(&job->sleep_timer, ns); > > + WITH_QEMU_LOCK_GUARD(&job_mutex) { > > + if (ns != -1) { > > + timer_mod(&job->sleep_timer, ns); > > + } > > + job->busy = false; > > + job_event_idle(job); > > Is this new macro safe to use in a coroutine context? Hi, I suppose it's safe. It would be nice to get some more opinions here. > > } > > - job->busy = false; > > - job_event_idle(job); > > - job_unlock(); > > qemu_coroutine_yield(); > > > > /* Set by job_enter_cond() before re-entering the > > coroutine. */ > > > > I haven't looked into WITH_QEMU_LOCK_GUARD before, I assume it's new. > If > it works like I think it does, this change seems good. > > (I'm assuming it works like a Python context manager and it drops > the > lock when it leaves the scope of the macro using GCC/Clang language > extensions.) >
On 30/09/20 14:15, Elena Afanasova wrote: >>> + WITH_QEMU_LOCK_GUARD(&job_mutex) { >>> + if (ns != -1) { >>> + timer_mod(&job->sleep_timer, ns); >>> + } >>> + job->busy = false; >>> + job_event_idle(job); >> Is this new macro safe to use in a coroutine context? > Hi, I suppose it's safe. It would be nice to get some more opinions > here. > Yes, the macro is just a wrapper around the qemu_mutex_lock/unlock functions (or qemu_co_mutex_lock/unlock depending on the type of its argument). Paolo
© 2016 - 2024 Red Hat, Inc.