Re: [Xen-devel] [PATCH 1 of 4 RFC] xl/remus : Network buffering setup helper functions

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-07-29 at 14:00 -0400, Shriram Rajagopalan wrote:
> On Mon, Jul 29, 2013 at 11:42 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
> wrote:
> > On Thu, 2013-07-25 at 00:09 -0700, Shriram Rajagopalan wrote:
> >> The functions contained in xl_netbuf.c are built on top of libnl3 API.
> >> They setup the necessary infrastructure required to for network output
> >> buffering in Remus.  In xend, the Remus python code performed this setup
> >> by invoking shell commands.  This code is built purely on top of APIs
> >> supplied by libnl3.0.
> >>
> >> There are two public helper functions:
> >> 1) remus_install_netbuf_on_dev to install a network buffer on a vif.
> >> Its basically equivalent to:
> >> a)find free ifb (say ifb0)
> >> b)ip link set ifb0 up
> >> c)tc qdisc add dev vif1.0 ingress
> >> d)tc filter add dev vif1.0 parent ffff: proto ip pref 10 u32 match u32 0 0 
> >> \
> >>       action mirred egress redirect dev ifb0
> >> e)tc qdisc add dev ifb0 root plug
> >> f)get handle to plug qdisc and control it programmatically after suspending
> >>   a VM/receiving checkpoint ack from remote
> >>
> >>
> >> 2) remus_uninstall_netbufs to remove all
> >> network buffers and ingress qdiscs from the supplied interface list.
> >>
> >> I have managed to code up 5 of the 6 steps mentioned above, purely in C, 
> >> by building
> >> on top of libnl3. There is currently no support in libnl3 to implement 
> >> step (d) (redirection)
> >> and it is pretty complicated to implement. So I am currently resorting to 
> >> system() lib call
> >> to get the job done.
> >>
> >> N.B. This implementation is just xl (cmdline utility) specific.
> >
> > You haven't included a Makefile change so I can't easily tell if this is
> > really part of the xl binary, or a separate helper or indeed part of
> > libxl.
> >
> 
> The Makefile changes were clumped together in patch 4, along with changes to
> configure.ac.

Please put these sorts of changes alongside the code changes, that might
mean reordering your series etc.

> >> Other toolstacks may choose
> >> to reuse this code or do this setup in their own way (scripts/shell 
> >> commands, etc.).
> >
> > It's not clear to me why this code wouldn't be part of libxl to be
> > called by xl and other toolstacks as needed. Why would a toolstack want
> > to do this differently?
> >
> > The point of libxl is precisely to relieve toolstacks of the burden of
> > doing these sorts of complex domain setup activities themselves by
> > putting them in a common library, I can't see why Remus setup would be
> > an exception to this.
> >
> > I think we need to answer this question before reviewing this code,
> > since doing it libxl would change the requirements of the code (from the
> > API exposed, to logging, to memory managment, to not using system
> > directly).
> >
> 
> If every one had to do this in the same way, then I agree that this
> code should be moved into libxl.
> 
> The setup can be done using a a few lines of shell script or in a 100-200
> line C code (like this patch).  Given that there is a choice, I didnt
> see any point
> in forcing all toolstacks to go the libxl way (resorting to its
> internal c code for setting
> up the buffers).  But if you think that we might as well make a sane
> choice and abstract
> this complexity from other toolstacks, I would be happy to push this
> into libxl itself.

If you think the sane choice is for everyone to do this the same way
then please do it in libxl.

Realistically I don't imagine other toolstacks will implement Remus
unless you either make it a no-brainer (by making it a trivial to use
libxl interface) or you do it for them.

Ian.


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