* [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard
@ 2020-09-29 13:42 Elena Afanasova
2020-09-29 18:04 ` John Snow
0 siblings, 1 reply; 4+ messages in thread
From: Elena Afanasova @ 2020-09-29 13:42 UTC (permalink / raw)
To: jsnow; +Cc: qemu-trivial, Elena Afanasova, qemu-devel, qemu-block
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard
2020-09-29 13:42 [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard Elena Afanasova
@ 2020-09-29 18:04 ` John Snow
2020-09-30 12:15 ` [PATCH] job: delete job_{lock,unlock} " Elena Afanasova
0 siblings, 1 reply; 4+ messages in thread
From: John Snow @ 2020-09-29 18:04 UTC (permalink / raw)
To: Elena Afanasova; +Cc: qemu-trivial, qemu-devel, qemu-block
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.)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] job: delete job_{lock,unlock} functions and replace them with lock guard
2020-09-29 18:04 ` John Snow
@ 2020-09-30 12:15 ` Elena Afanasova
2020-09-30 13:17 ` [PATCH] job: delete job_{lock, unlock} " Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Elena Afanasova @ 2020-09-30 12:15 UTC (permalink / raw)
To: John Snow; +Cc: qemu-trivial, pbonzini, qemu-devel, qemu-block
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.)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard
2020-09-30 12:15 ` [PATCH] job: delete job_{lock,unlock} " Elena Afanasova
@ 2020-09-30 13:17 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2020-09-30 13:17 UTC (permalink / raw)
To: Elena Afanasova, John Snow; +Cc: qemu-trivial, qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-30 13:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 13:42 [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard Elena Afanasova
2020-09-29 18:04 ` John Snow
2020-09-30 12:15 ` [PATCH] job: delete job_{lock,unlock} " Elena Afanasova
2020-09-30 13:17 ` [PATCH] job: delete job_{lock, unlock} " Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.