Re: [Xen-devel] [PATCH 3 of 4 RFC] xl/remus: Control network buffering in remus callbacks

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Wed, Aug 7, 2013 at 11:38 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:

>
> +#include <netlink/route/qdisc/plug.h>

We need a configure option to disable this, in case the host doesn't
have it.

 

>  #include <xc_dom.h>
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/hvm_xs_strings.h>
> @@ -1212,12 +1213,36 @@ int libxl__toolstack_save(uint32_t domid
>
>  /*----- remus callbacks -----*/
>
> +/* REMUS TODO: Issue disk checkpoint reqs. */
>  static int libxl__remus_domain_suspend_callback(void *data)

...

> +    if (!remus_ctx->num_netbufs) return is_suspended;
> +
> +    if (is_suspended) {
> +        for (i = 0; i < remus_ctx->num_netbufs; ++i) {
> +            ret = rtnl_qdisc_plug_buffer(remus_ctx->netbuf_qdisc_list[i]);
> +            if (!ret)
> +                ret = rtnl_qdisc_add(remus_ctx->nlsock, remus_ctx->netbuf_qdisc_list[i],

Needs the line length reducing to 70-75 characters, maximum.
(Multiple occcurrences of this problem in the patch.)

> @@ -1255,13 +1279,104 @@ static void libxl__remus_domain_checkpoi
>  static void remus_checkpoint_dm_saved(libxl__egc *egc,
>                                        libxl__domain_suspend_state *dss, int rc)
>  {
> -    /* REMUS TODO: Wait for disk and memory ack, release network buffer */
> -    /* REMUS TODO: make this asynchronous */
> -    assert(!rc); /* REMUS TODO handle this error properly */

...
> +    assert(!rc);

Does this TODO not need to remain ?

> +            if (!ret)
> +                ret = rtnl_qdisc_add(remus_ctx->nlsock, remus_ctx->netbuf_qdisc_list[i],
> +                                     NLM_F_REQUEST);
> +            if (ret) {
> +                LOG(ERROR, "Cannot release buffer from %s:%s",
> +                    dss->remus->netbuf_iflist[i], nl_geterror(ret));
> +                ret= 0;

Missing space after "=".

...

> +    usleep(dss->remus_ctx->interval * 1000);

This is still pretty bad, surely ?

Was thinking of updating this in a separate patch.. But I guess since this line

is changing, might as well do it in this patch.

 

> +    rtnl_link_put(ifb);

> +    return remus_ctx;
> +
> + end:
> +    if (ifb) rtnl_link_put(ifb);
> +    if (qdisc_cache) nl_cache_free(qdisc_cache);
> +    if (nlsock) nl_close(nlsock);

I think this can perhaps leak remus_ctx and qdisc.

There is no issue of leaking qdisc. Because it simply obtains references from

qdisc_cache, which is freed if things fail. 

WRT remus_ctx, I had intentionally skipped the free call. 

 Its allocated in the GC context. [the same one used by domain_suspend, etc].

And if this call fails, then the parent call libxl_domain_remus_start will fail.

I assumed that when libxl_free_all was called at this point, remus_ctx would also be

freed. Am I missing something in the overall control flow, that would cause this leak ?

Thanks,
Ian.

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