[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.