Re: [Qemu-devel] [PATCH RFC] monitor: temporary fix for dead-lock on event recursion

[Marc-André Lureau]

Hi

On Tue, Jul 31, 2018 at 9:05 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> With a Spice port chardev, it is possible to reenter
>> monitor_qapi_event_queue() (when the client disconnects for
>> example). This will dead-lock on monitor_lock.
>>
>> Instead, use some TLS variables to check for recursion and queue the
>> events.
> [...]
>>
>> Note that error report is now moved to the first caller, which may
>> receive an error for a recursed event. This is probably fine (95% of
>> callers use &error_abort, the rest have NULL error and ignore it)
>
> I'm not 100% sure I understand this paragraph, but it might be moot; see
> [*] below.
>
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  monitor.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index d8d8211ae4..d580c5a79c 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -633,7 +633,7 @@ static void monitor_qapi_event_handler(void *opaque);
>>   * applying any rate limiting if required.
>>   */
>>  static void
>> -monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>> +monitor_qapi_event_queue_no_recurse(QAPIEvent event, QDict *qdict, Error 
>> **errp)
>>  {
>>      MonitorQAPIEventConf *evconf;
>>      MonitorQAPIEventState *evstate;
>> @@ -688,6 +688,55 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, 
>> Error **errp)
>>      qemu_mutex_unlock(&monitor_lock);
>>  }
>>
>> +static void
>> +monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    /*
>> +     * If the function recurse, monitor_lock will dead-lock.
>> +     * Instead, queue pending events in TLS.
>
> recurses
>
> The claim is correct before the patch.  But the comment needs to explain
> current code.  Perhaps:
>
>         * monitor_qapi_event_queue_no_recurse() is not reentrant: it
>         * would deadlock on monitor_lock.  Work around by queueing
>         * events in thread-local storage.

ok

>
>> +     * TODO: remove this, make it re-enter safe.
>> +     */
>> +    static __thread bool recurse;
>> +    typedef struct MonitorQapiEvent {
>> +        QAPIEvent event;
>> +        QDict *qdict;
>> +        QSIMPLEQ_ENTRY(MonitorQapiEvent) entry;
>> +    } MonitorQapiEvent;
>> +    MonitorQapiEvent *ev;
>> +    static __thread QSIMPLEQ_HEAD(, MonitorQapiEvent) event_queue;
>
> Let's put the static variables first.

ok

>> +
>> +    if (!recurse) {
>> +        QSIMPLEQ_INIT(&event_queue);
>> +    }
>> +
>> +    ev = g_new(MonitorQapiEvent, 1);
>> +    ev->qdict = qobject_ref(qdict);
>> +    ev->event = event;
>> +    QSIMPLEQ_INSERT_TAIL(&event_queue, ev, entry);
>> +    if (recurse) {
>> +        return;
>> +    }
>> +
>> +    recurse = true;
>> +
>> +    while ((ev = QSIMPLEQ_FIRST(&event_queue)) != NULL) {
>> +        QSIMPLEQ_REMOVE_HEAD(&event_queue, entry);
>
> Could we use QSIMPLEQ_FOREACH_SAFE()?

I don't think so, the next variable could be NULL, while a recursive
call could be adding an element.

>
>> +        if (!local_err) {
>> +            monitor_qapi_event_queue_no_recurse(ev->event, ev->qdict,
>> +                                                &local_err);
>> +        }
>
> [*] This looks scary: we silently throw away events after event queuing
> fails.
>
> Fortunately, monitor_qapi_event_queue_no_recurse() cannot fail.  It
> takes an Error ** parameter only so you can put it into @qmp_emit.
>

right

> Aside: I wish we'd get rid of the indirection through @qmp_emit.
>

for later: You would hard code a function name instead?

> Let's pass &error_abort and drop @local_err.

actually, we can just ignore errp, right?

for later: What about dropping error from the emit function?

>
>> +        qobject_unref(ev->qdict);
>> +        g_free(ev);
>> +    }
>> +
>> +    recurse = false;
>
> Aha: @recurse means we've reentered the function.
>
> "Recurse" is imperative mood.  Misleading, as it's not an order to
> recurse.
>
> Rename to @recursed?  @reentered?
>

ok, let's rename it "reentered"

>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +}
>> +
>>  /*
>>   * This function runs evconf->rate ns after sending a throttled
>>   * event.
>
> monitor_lock clearly needs a rethink.  Your TODO comment suggests you
> agree.  But that's something for 3.1.
>
> I hate messing with locks at -rc3, but I also hate shipping known
> deadlocks.  Your patch isn't pretty, but probably as simple as we can
> make it for 3.0.  A few more review eyeballs would be nice.
>

thanks, I'll send v2

-- 
Marc-André Lureau