[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 |