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

[Elena Afanasova <eafanasova@gmail.com>]

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