On Sat, Aug 12, 2017 at 1:15 AM, Ben Pfaff <[email protected]> wrote: > 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? This patch and 8c2c225e481d ("netdev: Fix netdev_open() to track and recreate classless interfaces") may fix the same bug and I explained it in the commit message. I guess we should set suitable type to netdev_open() even the patch 8c2c225e481d("netdev: Fix netdev_open() to track and recreate classless interfaces") has been applied. If we don't need it, this is fine to me.
> 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. yes, if this patch is ok, I will send v3. > 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 <[email protected]> >> 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 <[email protected]> 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 <[email protected]> >> >> >> >> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
