Re: [Xen-devel] [XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for QMP socket access

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[XEN PATCH for-4.13 v2 6/6] libxl_qmp: Have a lock for 
QMP socket access"):
> Background:
>     This happens when attempting to create a guest with multiple
>     pci devices passthrough, libxl creates one connection per device to
>     attach and execute connect() on all at once before any single
>     connection has finished.
> 
> To work around this, we use a new lock.

Thanks again for tackling this.

> Error handling of connect() and lock() is a bit awkward as
> libxl__ev_qmp_send() doesn't allow to call the callback synchronously.
> So we setup a timer to have a callback that has been called
> asynchronously. We use the _abs variant it does strictly less than
> _rel, thus avoiding unnecessary code that could return an error
> (unnecessary because we only need to have the callback been called
> ASAP).

I have some problems with the approach here, I'm afraid.

> This patch workaround the fact that it's not possible to connect
> multiple time to a single QMP socket. QEMU listen on the socket with
> a backlog value of 1, which mean that on Linux when concurrent thread
> call connect() on the socket, they get EAGAIN.
...
>   * qmp_state     External   cfd    efd     id     rx_buf* tx_buf* msg*
>   * disconnected   Idle       NULL   Idle    reset  free    free    free
> + * waiting_lock   Active     open   Idle    reset  used    free    set
>   * connecting     Active     open   IN      reset  used    free    set
>   * cap.neg        Active     open   IN|OUT  sent   used    cap_neg set
>   * cap.neg        Active     open   IN      sent   used    free    set

Don't `lock' and `time' need to be added to this table ?
The table may become rather wide :-/.  Maybe it could be
compressed/abbreviated some more or maybe we'll just live with it
becoming wider.

>  out:
> -    return rc;
> +    /* An error occurred and we need to let the caller know.  At this
> +     * point, we can only do so via the callback. Unfortunately, the
> +     * callback of libxl__ev_slowlock_lock() might be called synchronously,
> +     * but libxl__ev_qmp_send() promise that it will not call the callback
> +     * synchronously. So we have to arrange to call the callback
> +     * asynchronously. */
> +    ev->rc = rc;
> +    struct timeval now = { 0 };
> +    rc = libxl__ev_time_register_abs(ev->ao, &ev->etime,
> +                                     lock_error_callback, now);
> +    /* If setting up the timer failed, there is no way to tell the caller
> +     * of libxl__ev_qmp_send() that the connection to the QMP socket
> +     * failed. But they are supposed to have a timer of their own. */
> +    if (rc)
> +        LOGD(ERROR, ev->domid,
> +             "Failed to setup a callback call. rc=%d", rc);

I don't think this is right.  I think this callback has to be set up
in a way that can't fail.  But I think this is possible.  We're free
to allocate memory (although this may not be needed) and the egc
(which we have) can contain callback pointers.

I can see two approaches:

 1. Invent a new libxl_ev_immediate_but_not_reentrant_callback (can't
    think of a good name right now).  Add a new list head to the egc,
    and when you want to register a callback, put it on that list.
    Add the callback execution to egc_run_callbacks, probably at the
    top in a loop (since immediate callbacks may add more immediate
    callbacks).

 2. Use libxl__ev_time; provide libxl__ev_time_register_now which
    cannot fail: it sets the abs time to 0 and skips the abortable
    stuff, and puts the libxl__ev_time on the front of the CTX etimes
    list.  Have egc_run_callbacks run immediate timeouts: ie, if the
    head of the etimes list has abs time of 0, strip it, and call it.

    I think this involves some special handling of this case because
    you want to avoid messing about with the OSEVENT_HOOKs, so
    you probably need a new bit in libxl__ev_time.  Otherwise
    time_deregister wouldn't know what to do.

I think 1 is probably less confusing and less risky.  It doesn't
disturb, or get entangled in, the existing ev_time code; it doesn't
involve pointlessly allocating an unused abortable.  And it doesn't
involve confusion over which error returns are possible where and
which rc values must be kept and which discarded.

Sorry,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel