I don't know.  This patch seems to say that it fixes a problem if we
revert commit 8c2c225e481d ("netdev: Fix netdev_open() to track and
recreate classless interfaces").  That patch hasn't been reverted.
Given that, can you explain the value of this patch?

Is removing route_table_reset() related to the rest of the patch?  If it
is related, please add the explanation to the commit message.  If it is
not related, then please submit it as a separate patch with the
explanation that you gave below.

I spent a few minutes playing with the style of this patch.  I'm
appending it in a style closer to what we usually prefer.  This should
not have changed the behavior at all.

I wonder whether we should change the behavior of netdev_open() with a
NULL type, so that it somehow searches for the proper type.

Thank you for your work making OVS better.

On Fri, Aug 11, 2017 at 06:02:06PM +0800, Tonghao Zhang wrote:
> Hi Ben, this patch is ok ?
> 
> On Fri, Aug 4, 2017 at 10:45 AM, Tonghao Zhang <xiangxia.m....@gmail.com> 
> wrote:
> > We can avoid the deadlock via  removing the route_table_reset() from
> > the route_table_init()
> >
> > the call trace is below
> > dp_initialize (ovsthread_once)
> >     route_table_init
> >         route_table_reset
> >              route_table_handle_msg
> >                  ovs_router_insert__
> >
> > ovs_router_insert__
> >        get_src_addr
> >             get_netdev_type
> >                    dp_enumerate_types
> >                          dp_initialize (ovsthread_once)
> >
> >
> > After removing the route_table_reset() from the route_table_init(),
> > the ovs-router works well. Because we run the route_table_run()
> > periodically, and route_table_valid is inited as false (we will call
> > the route_table_reset in this case). So it’s also unnecessary to
> > immediately  reset route table in the route_table_init().
> >
> > On Fri, Aug 4, 2017 at 2:00 AM, Ben Pfaff <b...@ovn.org> wrote:
> >> On Tue, Jul 18, 2017 at 08:44:15PM -0700, Tonghao Zhang wrote:
> >>> ovs-router module uses the netdev_open() to get routes.
> >>> But this module always calls the netdev_open() with type
> >>> which is NULL. This module may open the eth0, vethx,
> >>> vxlan_sys_4789, br0 if these network devices exist. And
> >>> these device will be opened as 'system' type.
> >>>
> >>> When debugging, somewhere netdev_ref it.  After reverting
> >>> "netdev: Fix netdev_open() to adhere to class type if given",
> >>> and when we call the 'ovs-appctl dpctl/show' or 'ovs-dpctl show',
> >>> the info is shown as below. the vxlan_sys_4789 is up
> >>> (eg. ifconfig vxlan_sys_4789 up).
> >>>
> >>> $ ovs-dpctl show
> >>> system@ovs-system:
> >>>       lookups: hit:4053 missed:118 lost:3
> >>>       flows: 0
> >>>       masks: hit:4154 total:1 hit/pkt:1.00
> >>>       port 0: ovs-system (internal)
> >>>       port 1: user-ovs-vm (internal)
> >>>       port 2: vxlan_sys_4789 (vxlan)
> >>>
> >>> But the info should be as below.
> >>> $ ovs-dpctl show
> >>> system@ovs-system:
> >>>       lookups: hit:4053 missed:118 lost:3
> >>>       flows: 0
> >>>       masks: hit:4154 total:1 hit/pkt:1.00
> >>>       port 0: ovs-system (internal)
> >>>       port 1: user-ovs-vm (internal)
> >>>       port 2: vxlan_sys_4789 (vxlan: packet_type=ptap)
> >>>
> >>> Because the netdev-class of 'system' type does not have the
> >>> 'get_config', and tunnel vports have 'get_config', then we can
> >>> get the config info(eg. packet_type=ptap) of tunnel vports.
> >>>
> >>> If we only revert the patch, there is a bug all the time. The
> >>> patch which Eelco support is fine to me. That patch avoid issue.
> >>> URL: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335560.html
> >>> But without it, the patch I support also avoid the problem.
> >>>
> >>> However we should check the type in the ovs-router module, this
> >>> patch works well with the patch Eelco support.
> >>>
> >>> Signed-off-by: Tonghao Zhang <xiangxia.m....@gmail.com>
> >>
> >> Thanks for the bug fix.
> >>
> >> This patch seems fine but can you help me understand the change to
> >> route_table_init()?  It isn't obviously connected to the rest of the
> >> patch.
> >>
> >> Thanks,
> >>
> >> Ben.
> >>
> >>> diff --git a/lib/route-table.c b/lib/route-table.c
> >>> index 67fda31..fc6845f 100644
> >>> --- a/lib/route-table.c
> >>> +++ b/lib/route-table.c
> >>> @@ -112,7 +112,6 @@ route_table_init(void)
> >>>          nln_notifier_create(nln, RTNLGRP_IPV6_ROUTE,
> >>>                              (nln_notify_func *) route_table_change, 
> >>> NULL);
> >>>
> >>> -    route_table_reset();
> >>>      name_table_init();
> >>>
> >>>      ovs_mutex_unlock(&route_table_mutex);

--8<--------------------------cut here-------------------------->8--

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index ce2f80b387a7..bb32d2855397 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -45,6 +45,7 @@
 #include "util.h"
 #include "unaligned.h"
 #include "openvswitch/vlog.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(ovs_router);
 
@@ -138,6 +139,48 @@ static void rt_init_match(struct match *match, uint32_t 
mark,
     match->flow.pkt_mark = mark;
 }
 
+/* Return the network device type of the specified
+ * 'netdev_name' if successful, otherwise null.
+ *
+ * The caller should free it.
+ * */
+static char *
+get_netdev_type(const char *netdev_name)
+{
+    struct sset dpif_names = SSET_INITIALIZER(&dpif_names);
+    struct sset dpif_types = SSET_INITIALIZER(&dpif_types);
+    char *netdev_type = NULL;
+
+    dp_enumerate_types(&dpif_types);
+
+    const char *dpif_type;
+    SSET_FOR_EACH (dpif_type, &dpif_types) {
+        if (dp_enumerate_names(dpif_type, &dpif_names)) {
+            continue;
+        }
+
+        const char *dpif_name;
+        SSET_FOR_EACH (dpif_name, &dpif_names) {
+            struct dpif *dpif;
+            if (!dpif_open(dpif_name, dpif_type, &dpif)) {
+                struct dpif_port dpif_port;
+                if (!dpif_port_query_by_name(dpif, netdev_name, &dpif_port)) {
+                    netdev_type = xstrdup(dpif_port.type);
+                    dpif_close(dpif);
+                    goto out;
+                }
+                dpif_close(dpif);
+            }
+        }
+    }
+
+out:
+    sset_destroy(&dpif_names);
+    sset_destroy(&dpif_types);
+
+    return netdev_type;
+}
+
 static int
 get_src_addr(const struct in6_addr *ip6_dst,
              const char output_bridge[], struct in6_addr *psrc)
@@ -147,7 +190,9 @@ get_src_addr(const struct in6_addr *ip6_dst,
     struct netdev *dev;
     bool is_ipv4;
 
-    err = netdev_open(output_bridge, NULL, &dev);
+    char *netdev_type = get_netdev_type(output_bridge);
+    err = netdev_open(output_bridge, netdev_type, &dev);
+    free(netdev_type);
     if (err) {
         return err;
     }
diff --git a/lib/route-table.c b/lib/route-table.c
index 67fda317638c..fc6845f831ff 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -112,7 +112,6 @@ route_table_init(void)
         nln_notifier_create(nln, RTNLGRP_IPV6_ROUTE,
                             (nln_notify_func *) route_table_change, NULL);
 
-    route_table_reset();
     name_table_init();
 
     ovs_mutex_unlock(&route_table_mutex);
-- 
2.10.2

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to