|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3 of 5 V2] tools/libxl: setup/teardown Remus network buffering
On Thu, 2013-08-29 at 15:16 -0700, Shriram Rajagopalan wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@xxxxxxxxx>
> # 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@xxxxxxxxx>
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@xxxxxxxxx>
> + *
> + * 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |