Re: [Xen-devel] [PATCH v10 4/5] remus: implement remus network buffering for nic devices

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH v10 4/5] remus: implement remus network buffering 
for nic devices"):
> 1.Add two members in libxl_domain_remus_info:

Thanks for this patch.

I'm deferring reviewing the parts of this inside libxl which use the
new libxl device interface, until we have the API documentation
comments which I discussed in my last email.  I hope that's OK.

But there i

>     netbuf: whether netbuf is enabled
>     netbufscript: the path of the script which will be run to setup
>        and tear down the guest's interface.
> 2.introduces remus-netbuf-setup hotplug script responsible for
>   setting up and tearing down the necessary infrastructure required for
>   network output buffering in Remus.  This script is intended to be invoked
>   by libxl for each guest interface, when starting or stopping Remus.
> 
>   Apart from returning success/failure indication via the usual hotplug
>   entries in xenstore, this script also writes to xenstore, the name of
>   the IFB device to be used to control the vif's network output.
> 
>   The script relies on libnl3 command line utilities to perform various
>   setup/teardown functions. The script is confined to Linux platforms only
>   since NetBSD does not seem to have libnl3.
> 
>   The following steps are taken during init:
>     a) establish a dedicated remus context containing libnl related
>        state (netlink sockets, qdisc caches, etc.,)
> 
>   The following steps are taken for each vif during setup:
>     a) call the hotplug script to setup its network buffer
> 
>     b) Obtain handles to plug qdiscs installed on the IFB devices
>        chosen by the hotplug scripts.
> 
>   And during teardown, the netlink resources are released, followed by
>   invocation of hotplug scripts to remove the ifb devices.
> 3.implement the remus device interface. setup, teardown, etc.
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>
> Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
> Reviewed-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
> ---
>  docs/misc/xenstore-paths.markdown      |   4 +
>  tools/hotplug/Linux/Makefile           |   1 +
>  tools/hotplug/Linux/remus-netbuf-setup | 183 ++++++++++++
>  tools/libxl/libxl.c                    |  18 ++
>  tools/libxl/libxl.h                    |  13 +
>  tools/libxl/libxl_internal.h           |   3 +
>  tools/libxl/libxl_netbuffer.c          | 519 
> +++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_nonetbuffer.c        |  67 +++++
>  tools/libxl/libxl_remus_device.c       |  22 +-
>  tools/libxl/libxl_types.idl            |   2 +
>  10 files changed, 831 insertions(+), 1 deletion(-)
>  create mode 100644 tools/hotplug/Linux/remus-netbuf-setup
> 



> diff --git a/docs/misc/xenstore-paths.markdown 
> b/docs/misc/xenstore-paths.markdown
> index 70ab7f4..039eaea 100644
> --- 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.
> +

Thanks for updating the doc.  Your changes to the hotplug Makefile
look good too.

> diff --git a/tools/hotplug/Linux/remus-netbuf-setup 
> b/tools/hotplug/Linux/remus-netbuf-setup
> new file mode 100644
> index 0000000..aed2583
> --- /dev/null
> +++ b/tools/hotplug/Linux/remus-netbuf-setup
> @@ -0,0 +1,183 @@
> +#!/bin/bash
> +#============================================================================
> +# ${XEN_SCRIPT_DIR}/remus-netbuf-setup
> +#
> +# Script for attaching a network buffer to the specified vif (in any mode).
> +# The hotplugging system will call this script when starting remus via libxl
> +# API, libxl_domain_remus_start.

Right.  Thanks for the comprehensive head comment.

> +#============================================================================
...
> +setup_ifb() {
> +
> +    for ifb in `ifconfig -a -s|egrep ^ifb|cut -d ' ' -f1`
> +    do
> +        local installed=`nl-qdisc-list -d $ifb`
> +        [ -n "$installed" ] && continue
> +        IFB="$ifb"
> +        break
> +    done

As far as I can see this attempts to search for an ifb which is not in
use.

I see you claim a lock to ensure that you don't fail due to races with
other copies of this script.

But are there potentially other things (not Xen related, parhaps) in
the system which might try to allocate an ifb using a similar
approach ?  How do we deal with the potential race with them ?

Also: I think you should:
 - write the IFB name to xenstore _before_ starting to configure it
 - in the loop I quote above, check in xenstore that the ifb is not
   in use by another domain

Otherwise there seems to be the following risk:
 1. You pick ifbX using the loop above
 2. You start to configure ifbX, eventually resulting in a
    configuration which makes it not show up as free
 3. Something bad happens and you fail, before writing the
    ifb name to xenstore

In this case, the ifb would be leaked.  (I see you do try to avoid
this with xs_write_failed, but scripts can fail for other reasons.)

> +    do_or_die tc qdisc add dev "$vif" ingress

I'm not qualified to review these tc manipulations.  I guess I'm going
to trust that they're correct.

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 80947c3..db30a97 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -437,6 +437,19 @@
>  #define LIBXL_HAVE_DRIVER_DOMAIN_CREATION 1
>  
>  /*
> + * LIBXL_HAVE_REMUS_NETBUF 1
> + *
> + * If this is defined, then the libxl_domain_remus_info structure will
> + * have a boolean field (netbuf) and a string field (netbufscript).
> + *
> + * netbuf, if true, indicates that network buffering should be enabled.
> + *
> + * netbufscript, if set, indicates the path to the hotplug script to
> + * setup or teardown network buffers.
> + */
> +#define LIBXL_HAVE_REMUS_NETBUF 1

Good.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 4278a6b..50bf1ef 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -566,6 +566,8 @@ libxl_domain_remus_info = Struct("domain_remus_info",[
>      ("interval",     integer),
>      ("blackhole",    bool),
>      ("compression",  bool),
> +    ("netbuf",       bool),
> +    ("netbufscript", string),

I think netbuf should be a defbool, not a bool.  Indeed, perhaps this
is true of the other options too.  Is there some reason it shouldn't
default to enabled ?

You should mention in your commit message that this is going to be
plumbed into xl and the documentation in the next patch.

Regarding the other remus options here (and perhaps changing their
types), I think it would be OK to break API compatibility, since the
previous versions of remus exposed via xl have not been suitable for
deployment.  Do you agree ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel