Re: [RFC PATCH 1/2] spapr: drc: Add support for async hcalls at the drc level

[Shivaprasad bhat]

Thanks for the comments Bharata.

On Fri, Nov 27, 2020 at 11:12 AM Bharata B Rao <bharata@linux.ibm.com> wrote:
>
...
> > +    qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > +    if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > +        error_report_err(err);
> > +        g_free(state);
> > +        return 0;
> > +    }
>
> Returning w/o releasing the lock.

Ah, right.. Will fix it.

>
> > +
> > +    if (!token) /* Token should be non-zero */
> > +        goto retry;
...
> > +    SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > +    qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +    QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > +        if (state->continue_token == token) {
> > +            state->func = func;
> > +            state->data = data;
> > +            qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > +                               spapr_drc_async_hcall_runner, state,
> > +                               QEMU_THREAD_JOINABLE);
> > +            break;
> > +        }
> > +    }
>
> Looks like QLIST_FOREACH should be enough here as you don't
> seem to be removing any list entry in this path.

okay.

>
> > +    qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + *      Waits for all pending async requests to complete
> > + *      thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > +    SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > +    if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > +        return;
> > +    }
> > +
> > +    QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > +        qemu_thread_join(&state->thread);
> > +        QLIST_REMOVE(state, node);
> > +        g_free(state);
> > +    }
> > +}
>
> Why is it safe to iterate the list here w/o the lock?

This is called in the reset path, the chances that a previous hcall to
check pending status is still running at this stage is extremely low.
I can still hold the lock to be safe though.

>
> Regards,
> Bharata.