Re: [Xen-devel] [PATCH 19/24] libxl: provide progress reporting for long-running operations

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-04-16 at 18:18 +0100, Ian Jackson wrote:
> This will be used for reporting, during domain creation, that the
> console is ready.

I think of progress as being 1%, 2%, 3%...10% etc type stuff. I guess
progress in the sense you mean here could be thought of as "milestones"?

> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 2f559d5..3795654 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -899,12 +899,25 @@ static void egc_run_callbacks(libxl__egc *egc)
>  {
>      EGC_GC;
>      libxl_event *ev, *ev_tmp;
> +    libxl__aop_occurred *aop, *aop_tmp;
> 
>      LIBXL_TAILQ_FOREACH_SAFE(ev, &egc->occurred_for_callback, link, ev_tmp) {
>          LIBXL_TAILQ_REMOVE(&egc->occurred_for_callback, ev, link);
>          CTX->event_hooks->event_occurs(CTX->event_hooks_user, ev);
>      }

Are the BSD FOREACH_SAFE functions safe against manipulation from other
threads?

I only ask because Stefano was surprised that this wasn't the case for
the Linux macros, in that context "SAFE" just means you can delete the
element in this thread (like you do here) but that if another thread is
also manipulating the same list.

Quoting Dan in<20120420113557.GJ27101@mwanda>:
> It's safe against deletion in the same thread.  But not against
> deletion from another thread.
> 
> At the beginning of the loop it stores a pointer to the next
> element.  If you delete the element you are on, no problem because
> you already have a pointer to the next one.  But if another thread
> deletes the next element, now you have a pointer which is wrong.

By my reading these macros suffer from the same and therefore a bunch
more locking is needed. I hope I'm mistaken...

> 
> +    LIBXL_TAILQ_FOREACH_SAFE(aop, &egc->aops_for_callback, entry, aop_tmp) {
> +        LIBXL_TAILQ_REMOVE(&egc->aops_for_callback, aop, entry);
> +        aop->how->callback(CTX, aop->ev, aop->how->for_callback);
> +
> +        CTX_LOCK;
> +        aop->ao->progress_reports_outstanding--;
> +        libxl__ao_complete_check_progress_reports(egc, aop->ao);
> +        CTX_UNLOCK;
> +
> +        free(aop);
> +    }
> +
>      libxl__ao *ao, *ao_tmp;
>      LIBXL_TAILQ_FOREACH_SAFE(ao, &egc->aos_for_callback,
>                               entry_for_callback, ao_tmp) {
> @@ -1285,6 +1298,7 @@ void libxl__ao_abort(libxl__ao *ao)
>      assert(ao->magic == LIBXL__AO_MAGIC);
>      assert(ao->in_initiator);
>      assert(!ao->complete);
> +    assert(!ao->progress_reports_outstanding);
>      libxl__ao__destroy(CTX, ao);
>  }
> 
> @@ -1295,6 +1309,23 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao 
> *ao, int rc)
>      ao->complete = 1;
>      ao->rc = rc;
> 
> +    libxl__ao_complete_check_progress_reports(egc, ao);
> +}
> +
> +void libxl__ao_complete_check_progress_reports(libxl__egc *egc, libxl__ao 
> *ao)
> +{
> +    /* We don't consider an ao complete if it has any outstanding
> +     * callbacks.  These callbacks might be outstanding on other
> +     * threads, queued up in the other threads' egc's.  Those threads
> +     * will, after making the callback, take out the lock again,
> +     * decrememt progress_reports_outstanding, and call us again.

          decrement

> +     */
> +
> +    assert(ao->progress_reports_outstanding >= 0);
> +
> +    if (!ao->complete || ao->progress_reports_outstanding)
> +        return;
> +
>      if (ao->poller) {
>          assert(ao->in_initiator);
>          if (!ao->constructing)
> @@ -1316,34 +1347,6 @@ void libxl__ao_complete(libxl__egc *egc, libxl__ao 
> *ao, int rc)
>          libxl__ao__destroy(libxl__gc_owner(&egc->gc), ao);
>  }
> 
> @@ -1401,6 +1404,69 @@ int libxl__ao_inprogress(libxl__ao *ao)
>  }
> 
> 
> +/* progress reporting */
> +
> +/* The application indicates a desire to ignore events by passing NULL
> + * for how.  But we want to copy *how.  So we have this dummy function
> + * whose address is stored in callback if the app passed how==NULL. */
> +static void dummy_asyncprogress_callback_ignore
> +  (libxl_ctx *ctx, libxl_event *ev, void *for_callback) { }
> +
> +void libxl__ao_progress_gethow(libxl_asyncprogress_how *in_state,
> +                               const libxl_asyncprogress_how *from_app) {
> +    if (from_app)
> +        *in_state = *from_app;
> +    else
> +        in_state->callback = dummy_asyncprogress_callback_ignore;
> +}
> +
> +void libxl__ao_progress_report(libxl__egc *egc, libxl__ao *ao,
> +        const libxl_asyncprogress_how *how, libxl_event *ev)
> +{
> +    ev->for_user = how->for_event;
> +    if (how->callback == dummy_asyncprogress_callback_ignore) {
> +        /* ignore */
> +    } else if (how->callback) {
> +        libxl__aop_occurred *aop = libxl__zalloc(0, sizeof(*aop));
> +        ao->progress_reports_outstanding++;

Locking for this increment happens in the caller? (later: found the
comment which confirms this now).

> +        aop->ao = ao;
> +        aop->ev = ev;
> +        aop->how = how;
> +    } else {
> +        libxl__event_occurred(egc, ev);
> +    }
> +}
> +
> +
> +libxl__ao *libxl__ao_create(libxl_ctx *ctx, uint32_t domid,
> +                            const libxl_asyncop_how *how)

AFAICT this function has just moved and not changed, right?

> +{
> +    libxl__ao *ao;
> +
> +    ao = calloc(1, sizeof(*ao));
> +    if (!ao) goto out;
> +
> +    ao->magic = LIBXL__AO_MAGIC;
> +    ao->constructing = 1;
> +    ao->in_initiator = 1;
> +    ao->poller = 0;
> +    ao->domid = domid;
> +    LIBXL_INIT_GC(ao->gc, ctx);
> +
> +    if (how) {
> +        ao->how = *how;
> +    } else {
> +        ao->poller = libxl__poller_get(ctx);
> +        if (!ao->poller) goto out;
> +    }
> +    return ao;
> +
> + out:
> +    if (ao) libxl__ao__destroy(ctx, ao);
> +    return NULL;
> +}
> +
> +
>  /*
>   * Local variables:
>   * mode: C
[...]

My only concern is about the locking of the list walks -- but if that is
an issue it will also be there in existing code so I think it can all be
fixed in one go later. Hence:

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel