Re: [PATCH] job: delete job_{lock, unlock} functions and replace them with lock guard

[John Snow]

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.)