| Message ID | 20200929134214.4103-1-eafanasova@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: delete job_{lock, unlock} functions and replace them with lock guard | expand |
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
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. */
Signed-off-by: Elena Afanasova <eafanasova@gmail.com> --- job.c | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-)