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