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

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

Hi Ian,

  Thank you for the reply, I have some comments below...

On 07/10/2014 07:15 AM, Ian Jackson wrote:
> Hongyang Yang writes ("Re: [PATCH v10 4/5] remus: implement remus network buffering 
> for nic devices"):
> >     Sorry for the late reply, just notice this comment...
> And here I am being even later replying...
>
> > On 06/06/2014 01:24 AM, Ian Jackson wrote:
> > > 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 ?
> Have I missed an answer to this question ?
>
> > > 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
Sorry for mention this explicit,your comment here has already been addressed:
+#return 0 if the ifb is free
+check_ifb() {
+    local installed=`nl-qdisc-list -d $1`
+    [ -n "$installed" ] && return 1
+
+    for domid in `xenstore-list "/local/domain" 2>/dev/null || true`
+    do
+        [ $domid -eq 0 ] && continue
+        xenstore-exists "/libxl/$domid/remus/netbuf" || continue
+        for devid in `xenstore-list "/libxl/$domid/remus/netbuf" 2>/dev/null || 
true`
+        do
+            local path="/libxl/$domid/remus/netbuf/$devid/ifb"
+            xenstore-exists $path || continue
+            local ifb=`xenstore-read "$path" 2>/dev/null || true`
+            [ "$ifb" = "$1" ] && return 1
+        done
+    done
+
+    return 0
+}
+
+setup_ifb() {
+
+    for ifb in `ifconfig -a -s|egrep ^ifb|cut -d ' ' -f1`
+    do
+        check_ifb "$ifb" || continue
+        IFB="$ifb"
+        break
+    done
+
+    if [ -z "$IFB" ]
+    then
+        fatal "Unable to find a free IFB device for $vifname"
+    fi
+
+    #not using xenstore_write that automatically exits on error
+    #because we need to cleanup
+    _xenstore_write "$XENBUS_PATH/ifb" "$IFB" || xs_write_failed "$vifname" 
"$IFB"
+    do_or_die ip link set dev "$IFB" up
+}

> > > 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.)
> > If the setup failed for any reason, we will call teardown in the remus
> > device framework, so the ifb won't be leaked.
> I'm afraid that you can't assume that your script will necessarily
> execute the teardown.  Perhaps the script got killed by the OOM
> killer, or something else horrible went wrong.
>
> So you need to make sure that all the states the system goes through,
> however briefly and no matter what cleanup will be attempted on
> failure.
>
> > > > 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 ?
> > The netbuffering is enabled by default, this option is used to disable
> > the netbuffering support.
> Maybe I haven't followed the code here correctly, but I think that has
> to be done with a defbool.  Where is the default set ?  I don't see it
> in this patch.
It was set in the main_remus function in netbuf command switch patch,
default is on, and will only be switched off when user passes an -n option,
so I think this does not need to be done with a defbool:
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 68df548..b324f34 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7175,8 +7175,9 @@ int main_remus(int argc, char **argv)
     r_info.interval = 200;
     r_info.blackhole = 0;
     r_info.compression = 1;
+    r_info.netbuf = 1;

-    SWITCH_FOREACH_OPT(opt, "bui:s:e", NULL, "remus", 2) {
+    SWITCH_FOREACH_OPT(opt, "buni:s:N:e", NULL, "remus", 2) {
@@ -7186,6 +7187,12 @@ int main_remus(int argc, char **argv)
     case 'u':
         r_info.compression = 0;
         break;
+    case 'n':
+        r_info.netbuf = 0;
+        break;
+    case 'N':
> Thanks,
> Ian.
> .
--
Thanks,
Yang.

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