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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Shriram Rajagopalan writes ("[Xen-devel] [PATCH 3 of 4 RFC] xl/remus: Control 
network buffering in remus        callbacks"):
> This patch constitutes the core network buffering logic.
> Libxl would receive a list of IFB devices that collectively act as
> network buffering devices, for a given guest. Also, libxl expects that
> every ifb device in the list supplied by a toolstack (netbuf_iflist)
> should have a plug qdisc installed.

This seems to have many of the required pieces and is going in the
right direction.

> diff -r 3cd67f6ff63a -r bef729fc4336 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Thu Jul 25 00:02:19 2013 -0700
> +++ b/tools/libxl/libxl_dom.c Thu Jul 25 00:02:22 2013 -0700
> @@ -20,6 +20,7 @@
>  #include "libxl_internal.h"
>  #include "libxl_arch.h"
>  
> +#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 ?

> +static libxl__remus_ctx *setup_remus_ctx(libxl__gc *gc,
> +                                         const libxl_domain_remus_info 
> *r_info)
> +{
...
> +    libxl_string_list l;
...
> +    l = r_info->netbuf_iflist;

Is this a convenience shorthand ?  If so I think it might be better to
say
   const libxl_string_list *l = &r_info->netbuf_iflist;

...
> +    nl_connect(nlsock, NETLINK_ROUTE);

Does this not return an error value ?

> +    if (rtnl_qdisc_alloc_cache(nlsock, &qdisc_cache) < 0) {
> +        LOG(ERROR, "setup_remus_ctx: failed to allocate qdisc cache");
> +        goto end;
> +    }
> +
> +    remus_ctx->netbuf_qdisc_list = libxl__calloc(gc, num_netbufs + 1, 
> +                                                 sizeof(struct rtnl_qdisc 
> *));

Why num_netbufs+1 ?

> +    remus_ctx->num_netbufs = num_netbufs;
> +    remus_ctx->nlsock = nlsock;
> +    remus_ctx->qdisc_cache = qdisc_cache;

It would probably be better to use the fields in the remus_ctx
directly, rather than having a separate set of local variables.

> +    ifb = rtnl_link_alloc();
> +
> +    for (i = 0; i < num_netbufs; ++i) {
> +
> +        if (rtnl_link_get_kernel(nlsock, 0, l[i], &ifb) < 0) {
> +            LOG(ERROR, "setup_remus_ctx: cannot obtain handle for %s", l[i]);
> +            goto end;

Do these functions not provide errno values, or the moral equivalent ?

> +        }
> +
> +        ifindex = rtnl_link_get_ifindex(ifb);
> +        if (!ifindex) {
> +            LOG(ERROR, "setup_remus_ctx: invalid interface %s", l[i]);
> +            goto end;
> +        }
> +
> +        qdisc = rtnl_qdisc_get_by_parent(qdisc_cache, ifindex, TC_H_ROOT);
> +        if (!qdisc || strcmp(rtnl_tc_get_kind(TC_CAST(qdisc)), "plug")) {

Can rtnl_tc_get_kind fail ?

> +            LOG(ERROR, "setup_remus_ctx: plug qdisc is not installed on %s", 
> l[i]);
> +            goto end;
> +        }
> +
> +        remus_ctx->netbuf_qdisc_list[i] = qdisc;
> +    }
> +
> +    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.

Thanks,
Ian.

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