[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 05/10 V7] remus: Remus network buffering core and APIs to setup/teardown
Lai Jiangshan writes ("[PATCH 05/10 V7] remus: Remus network buffering core and APIs to setup/teardown"): Thanks. I have reviewed much of this in some detail and have some comments. > --- a/docs/misc/xenstore-paths.markdown > +++ b/docs/misc/xenstore-paths.markdown > @@ -385,6 +385,10 @@ The guest's virtual time offset from UTC in seconds. > > The device model version for a domain. > > +#### /libxl/$DOMID/remus/netbuf/$DEVID/ifb = STRING [n,INTERNAL] > + > +IFB device used by Remus to buffer network output from the associated vif. I think this should expand "IFB". Also, are these interface buffer devices really called "IFB" and not "ifb" ? > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index 84a467c..218f55e 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -52,6 +52,8 @@ else > LIBXL_OBJS-y += libxl_nonetbuffer.o > endif > > +LIBXL_OBJS-y += libxl_remus.o > + > LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o > LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 8d63f90..e3e9f6f 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -753,9 +753,6 @@ int libxl__toolstack_restore(uint32_t domid, const > uint8_t *buf, > > /*==================== Domain suspend (save) ====================*/ > > -static void domain_suspend_done(libxl__egc *egc, > - libxl__domain_suspend_state *dss, int rc); > - > /*----- complicated callback, called by xc_domain_save -----*/ > > /* > @@ -1508,8 +1505,8 @@ static void > save_device_model_datacopier_done(libxl__egc *egc, > dss->save_dm_callback(egc, dss, our_rc); > } > > -static void domain_suspend_done(libxl__egc *egc, > - libxl__domain_suspend_state *dss, int rc) > +void domain_suspend_done(libxl__egc *egc, > + libxl__domain_suspend_state *dss, int rc) > { > STATE_AO_GC(dss->ao); > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 2f64382..4006174 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -2313,6 +2313,23 @@ typedef struct libxl__remus_state { > > _hidden int libxl__netbuffer_enabled(libxl__gc *gc); > > +_hidden void domain_suspend_done(libxl__egc *egc, > + libxl__domain_suspend_state *dss, > + int rc); > + > +_hidden void libxl__remus_setup_done(libxl__egc *egc, > + libxl__domain_suspend_state *dss, > + int rc); > + > +_hidden void libxl__remus_netbuf_setup(libxl__egc *egc, > + libxl__domain_suspend_state *dss); > + > +_hidden void libxl__remus_teardown_done(libxl__egc *egc, > + libxl__domain_suspend_state *dss); > + > +_hidden void libxl__remus_netbuf_teardown(libxl__egc *egc, > + libxl__domain_suspend_state *dss); > + > struct libxl__domain_suspend_state { > /* set by caller of libxl__domain_suspend */ > libxl__ao *ao; > diff --git a/tools/libxl/libxl_netbuffer.c b/tools/libxl/libxl_netbuffer.c > index 8e23d75..2c77076 100644 > --- a/tools/libxl/libxl_netbuffer.c > +++ b/tools/libxl/libxl_netbuffer.c > @@ -17,11 +17,492 @@ > > #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_state { > + struct rtnl_qdisc **netbuf_qdisc_list; > + struct nl_sock *nlsock; > + struct nl_cache *qdisc_cache; > + const char **vif_list; > + const char **ifb_list; > + uint32_t num_netbufs; > + uint32_t unused; > +} libxl__remus_netbuf_state; > + > int libxl__netbuffer_enabled(libxl__gc *gc) > { > return 1; > } > > +/* If the device has a vifname, then use that instead of > + * the vifX.Y format. > + */ > +static const char *get_vifname(libxl__gc *gc, uint32_t domid, > + libxl_device_nic *nic) > +{ > + const char *vifname = NULL; > + const char *path; > + int rc; > + > + path = libxl__sprintf(gc, "%s/backend/vif/%d/%d/vifname", > + libxl__xs_get_dompath(gc, 0), domid, nic->devid); > + rc = libxl__xs_read_checked(gc, XBT_NULL, path, &vifname); > + if (!rc && !vifname) { > + /* use the default name */ > + vifname = libxl__device_nic_devname(gc, domid, > + nic->devid, > + nic->nictype); > + } > + > + return vifname; > +} I think the error handling here is rather odd. It would be better to use the "goto out" style. And the callers should treat NULL from this function as a fatal error. > +static const char **get_guest_vif_list(libxl__gc *gc, uint32_t domid, > + int *num_vifs) > +{ > + libxl_device_nic *nics = NULL; > + int nb, i = 0; > + const char **vif_list = NULL; > + > + *num_vifs = 0; > + nics = libxl_device_nic_list(CTX, domid, &nb); > + if (!nics) > + return NULL; It would be clearer IMO if this sayd "goto out"; > + > + /* Ensure that none of the vifs are backed by driver domains */ > + for (i = 0; i < nb; i++) { > + if (nics[i].backend_domid != LIBXL_TOOLSTACK_DOMID) { > + const char *vifname = get_vifname(gc, domid, &nics[i]); > + > + if (!vifname) > + vifname = "(unknown)"; > + LOG(ERROR, "vif %s has driver domain (%u) as its backend. " > + "Network buffering is not supported with driver domains", > + vifname, nics[i].backend_domid); > + *num_vifs = -1; > + goto out; The error handling return style of this function is very odd and not documented. > +static void free_qdiscs(libxl__remus_netbuf_state *netbuf_state) > +{ > + int i; > + struct rtnl_qdisc *qdisc = NULL; > + > + /* free qdiscs */ > + for (i = 0; i < netbuf_state->num_netbufs; i++) { > + qdisc = netbuf_state->netbuf_qdisc_list[i]; If you made this qdisc = &netbuf_state->netbuf_qdisc_list[i]; then you could say *qdisc = NULL; and not have to write out the long expression again. > + /* free qdisc cache */ > + if (netbuf_state->qdisc_cache) { > + nl_cache_clear(netbuf_state->qdisc_cache); > + nl_cache_free(netbuf_state->qdisc_cache); Wrong indent level. > +static int init_qdiscs(libxl__gc *gc, > + libxl__remus_state *remus_state) > +{ ... > + libxl__remus_netbuf_state * const netbuf_state = > remus_state->netbuf_state; ^ Coding style (extra space). > +static void netbuf_setup_timeout_cb(libxl__egc *egc, > + libxl__ev_time *ev, > + const struct timeval *requested_abs) > +{ > + libxl__remus_state *remus_state = CONTAINER_OF(ev, *remus_state, > timeout); > + > + /* Convenience aliases */ > + const int devid = remus_state->dev_id; > + libxl__remus_netbuf_state *const netbuf_state = > remus_state->netbuf_state; > + const char *const vif = netbuf_state->vif_list[devid]; > + > + STATE_AO_GC(remus_state->dss->ao); > + > + libxl__ev_time_deregister(gc, &remus_state->timeout); > + assert(libxl__ev_child_inuse(&remus_state->child)); > + > + LOG(DEBUG, "killing hotplug script %s (on vif %s) because of timeout", > + remus_state->netbufscript, vif); > + > + if (kill(remus_state->child.pid, SIGKILL)) { > + LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]", > + remus_state->netbufscript, > + (unsigned long)remus_state->child.pid); > + } > + > + return; > +} This function bears a striking resemblance to device_hotplug_timeout_cb. Likewise parts of exec_netbuf_script look very much like parts of device_hotplug, etc. You should arrange to reuse code rather than clone-and-hacking it, refactoring if necessary. If refactoring is necessary, that should be brought out into a pre-patch with no functional change. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |