Re: [PATCH 3 of 5 V2] tools/libxl: setup/teardown Remus network buffering

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2013-08-29 at 15:16 -0700, Shriram Rajagopalan wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@cs.ubc.ca>
> # Date 1377812196 25200
> # Node ID 3e8b0aa7cd4a945ec3efe14c969700c9e23ea2cc
> # Parent  79b21d778550f74e550af791eca41d4ca152492a
> tools/libxl: setup/teardown Remus network buffering
> 
> Setup/teardown remus network buffering for a given guest, when
> libxl_domain_remus_start API is invoked.
> 
> This patch does the following during setup:
>  a) call the hotplug script for each vif to setup its network buffer
> 
>  b) establish a dedicated remus context containing libnl related
>     state (netlink sockets, qdisc caches, etc.,)
> 
>  c) Obtain handles to plug qdiscs installed on the IFB devices
>     chosen by the hotplug scripts.
> 
> During teardown, the hotplug scripts are called to remove the IFB
> devices. This is followed by releasing netlink resources.
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

This could use feedback from Ian on the async bits and Roger on the
hotplug bits.

> 
> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/Makefile
> --- a/tools/libxl/Makefile	Thu Aug 29 14:36:25 2013 -0700
> +++ b/tools/libxl/Makefile	Thu Aug 29 14:36:36 2013 -0700
> @@ -40,6 +40,13 @@ LIBXL_OBJS-y += libxl_blktap2.o
>  else
>  LIBXL_OBJS-y += libxl_noblktap2.o
>  endif
> +
> +ifeq ($(CONFIG_REMUS_NETBUF),y)
> +LIBXL_OBJS-y += libxl_netbuffer.o
> +else
> +LIBXL_OBJS-y += libxl_nonetbuffer.o
> +endif
> +
>  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
>  LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o libxl_noarch.o
>  LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_noarch.o
> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Thu Aug 29 14:36:25 2013 -0700
> +++ b/tools/libxl/libxl.c	Thu Aug 29 14:36:36 2013 -0700
> @@ -692,8 +692,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *
>      return ptr;
>  }
>  
> -static void remus_failover_cb(libxl__egc *egc,
> -                              libxl__domain_suspend_state *dss, int rc);
> +static void remus_replication_failure_cb(libxl__egc *egc,
> +                                         libxl__domain_suspend_state *dss,
> +                                         int rc);
>  
>  /* TODO: Explicit Checkpoint acknowledgements via recv_fd. */
>  int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
> @@ -712,18 +713,44 @@ int libxl_domain_remus_start(libxl_ctx *
>  
>      GCNEW(dss);
>      dss->ao = ao;
> -    dss->callback = remus_failover_cb;
> +    dss->callback = remus_replication_failure_cb;
>      dss->domid = domid;
>      dss->fd = send_fd;
>      /* TODO do something with recv_fd */
>      dss->type = type;
>      dss->live = 1;
>      dss->debug = 0;
> -    dss->remus = info;
>  
>      assert(info);
>  
> -    /* TBD: Remus setup - i.e. attach qdisc, enable disk buffering, etc */
> +    GCNEW(dss->remus_ctx);
> +
> +    /* convenience shorthand */
> +    libxl__remus_ctx *remus_ctx = dss->remus_ctx;
> +    remus_ctx->blackhole = info->blackhole;
> +    remus_ctx->interval = info->interval;
> +    remus_ctx->compression = info->compression;
> +
> +    /* Setup network buffering before invoking domain_suspend */
> +    if (info->netbuf) {
> +        if (info->netbufscript) {
> +            remus_ctx->netbufscript =
> +                libxl__strdup(gc, info->netbufscript);
> +        } else {
> +            remus_ctx->netbufscript =
> +                libxl__sprintf(gc, "%s/remus-netbuf-setup",
> +                              libxl__xen_script_dir_path());

GCSPRINTF would help shorten this line.

(we don't seem to have GCSTRDUP to help the other side of the else, oh
well)

> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl_netbuffer.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/libxl/libxl_netbuffer.c	Thu Aug 29 14:36:36 2013 -0700
> @@ -0,0 +1,434 @@
> +/*
> + * Copyright (C) 2013
> + * Author Shriram Rajagopalan <rshriram@cs.ubc.ca>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +#include <netlink/cache.h>
> +#include <netlink/socket.h>
> +#include <netlink/attr.h>
> +#include <netlink/route/link.h>
> +#include <netlink/route/route.h>
> +#include <netlink/route/qdisc.h>
> +#include <netlink/route/qdisc/plug.h>
> +
> +typedef struct libxl__remus_netbuf_ctx {
> +    struct rtnl_qdisc **netbuf_qdisc_list;
> +    struct nl_sock *nlsock;
> +    struct nl_cache *qdisc_cache;
> +    char **vif_list;
> +    char **ifb_list;
> +    uint32_t num_netbufs;
> +    uint32_t unused;
> +} libxl__remus_netbuf_ctx;
> +
> +/* If the device has a vifname, then use that instead of
> + * the vifX.Y format.
> + */
> +static char *get_vifname(libxl__gc *gc, uint32_t domid,
> +                         libxl_device_nic *nic)
> +{
> +    char *vifname = NULL;
> +    vifname = libxl__xs_read(gc, XBT_NULL,
> +                             libxl__sprintf(gc,
> +                                            "%s/backend/vif/%d/%d/vifname",
> +                                            libxl__xs_get_dompath(gc, 0),
> +                                            domid, nic->devid));
> +    if (!vifname) {
> +        vifname = (char *)libxl__device_nic_devname(gc, domid,

Please don't cast away the const here. Either make this function return
const or dup this. Preferably const this function.

> +                                                    nic->devid,
> +                                                    nic->nictype);
> +    }
> +
> +    return vifname;
> +}
> +
> +static char **get_guest_vif_list(libxl__gc *gc, uint32_t domid,
> +                                 int *num_vifs)
> +{
> +    libxl_device_nic *nics;
> +    int nb, i = 0;
> +    char **vif_list = NULL;
> +
> +    nics = libxl_device_nic_list(CTX, domid, &nb);
> +    if (!nics) {
> +        *num_vifs = 0;
> +        return NULL;
> +    }
> +
> +    vif_list = libxl__calloc(gc, nb, sizeof(char *));
> +    for (i = 0; i < nb; ++i) {
> +        vif_list[i] = get_vifname(gc, domid, &nics[i]);
> +        libxl_device_nic_dispose(&nics[i]);
> +    }
> +    free(nics);
> +
> +    *num_vifs = nb;
> +    return vif_list;
> +}

I think rather than creating a list of char *'s you should just use the
array of libxl_device_nic's and pass that around in your context, pass
the individual elements to libxl__netbuf_script_setup etc and then let
them determine the name as necessary. That would seem to remove a bunch
of book keeping around this list.

> +static int libxl__netbuf_script_setup(libxl__gc *gc, uint32_t domid,
> +                                      int devid, char *vif, char *script,
> +                                      char **ifb)
> +{
> +    int rc;
> +    char *out_path_base, *hotplug_error;
> +
> +    rc = libxl__exec_netbuf_script(gc, domid, devid, vif,
> +                                   script, "setup");
> +    if (rc) return rc;
> +
> +    out_path_base = GCSPRINTF("%s/remus/netbuf/%d",
> +                              libxl__xs_libxl_path(gc, domid), devid);
> +
> +    /* Now wait for the result (XENBUS_PATH/hotplug-status).
> +     * It doesnt matter what the result is. If the hotplug-status path

"doesn't"

> +     * appears, then we are good to go.
> +     */
> +    rc = libxl__wait_for_offspring(gc, domid, LIBXL_HOTPLUG_TIMEOUT,
> +                                   script,
> +                                   /* path */
> +                                   GCSPRINTF("%s/hotplug-status",
> +                                             out_path_base),
> +                                   NULL /* state */,
> +                                   NULL, script_done_cb, NULL);

Does this wait synchronously? Doesn't it need to be async, with the
remainder of this function in the done_cb?

This is one of those places where Ian J needs to comment I think.

> +/* Scan through the list of vifs belonging to domid and
> + * invoke the netbufscript to setup the IFB device & plug qdisc
> + * for each vif. Then scan through the list of IFB devices to obtain
> + * a handle on the plug qdisc installed on these IFB devices.
> + * Network output buffering is controlled via these qdiscs.
> + */
> +int libxl__remus_netbuf_setup(libxl__gc *gc, uint32_t domid,
> +			      libxl__remus_ctx *remus_ctx)
> +{
> +    int i, ret, ifindex, num_netbufs = 0;
> +    struct rtnl_link *ifb = NULL;
> +    struct rtnl_qdisc *qdisc = NULL;
> +    libxl__remus_netbuf_ctx *netbuf_ctx = NULL;
> +
> +    /* If the domain backend is a stubdom, do nothing. We dont

"don't" (is your apostrophe key broken? ;-) )

> +     * support stubdom network buffering yet.
> +     */
> +    if (libxl_get_stubdom_id(CTX, domid)) {
> +        LOG(ERROR, "Network buffering is not supported with stubdoms");
> +        return ERROR_FAIL;
> +    }

This just checks for a HVM stub device model I think, whereas AIUI you
are trying to test if the NICs are backed by a driver domains?

For that case I think you need to check backend_domid for each NIC.

> +
> +    netbuf_ctx = libxl__zalloc(gc, sizeof(libxl__remus_netbuf_ctx));
> +    netbuf_ctx->vif_list = get_guest_vif_list(gc, domid, &num_netbufs);
> +    if (!num_netbufs) return 0;
> +
> +    netbuf_ctx->ifb_list = libxl__calloc(gc, num_netbufs,
> +                                         sizeof(char *));
> +
> +    /* convenience vars */
> +    char **vif_list = netbuf_ctx->vif_list;
> +    char **ifb_list = netbuf_ctx->ifb_list;

const on these?

> +
> +    for (i = 0; i < num_netbufs; ++i) {
> +
> +        /* The setup script does the following
> +         * find a free IFB to act as buffer for the vif.
> +         * set up the plug qdisc on the IFB.
> +         */
> +        ret = libxl__netbuf_script_setup(gc, domid, i, vif_list[i],
> +                                         (char *) remus_ctx->netbufscript,

Are you casting another const away here? Please don't.
[...]

I assume all this libnl stuff is right.


> diff -r 79b21d778550 -r 3e8b0aa7cd4a tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl	Thu Aug 29 14:36:25 2013 -0700
> +++ b/tools/libxl/libxl_types.idl	Thu Aug 29 14:36:36 2013 -0700
> @@ -526,6 +526,8 @@ libxl_domain_remus_info = Struct("domain
>      ("interval",     integer),
>      ("blackhole",    bool),
>      ("compression",  bool),
> +    ("netbuf",  bool),
> +    ("netbufscript", string),

Please align the types as in the lines above.

Ian