Hi Wenxu, First FYI you send your emails from [email protected] but the from in the header has [email protected]. Guess this is not a big problem, but for now, however, it’s causing the message to be partly blocked by our spam application :( Maybe it’s a misconfiguration on your side?
> From: wenxu <[email protected]> > > The netdev-offload in tc mode can't work with flow-restore-wait. > When the vswitchd restart with flow-restore-wait, the tc qdisc > will be delete in netdev_set_flow_api_enabled. The netdev flow > api can be enabled after the flow-restore-wait flag removing. > > Signed-off-by: wenxu <[email protected]> > --- > lib/netdev-offload.c | 6 ++++-- > lib/netdev-offload.h | 3 ++- > vswitchd/bridge.c | 13 +++++++++---- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index fb108c0..8547ce8 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -778,9 +778,11 @@ netdev_ports_flow_init(void) > } > > void > -netdev_set_flow_api_enabled(const struct smap *ovs_other_config) > +netdev_set_flow_api_enabled(const struct smap *ovs_other_config, > + bool flow_restore_wait) > { > - if (smap_get_bool(ovs_other_config, "hw-offload", false)) { > + if (smap_get_bool(ovs_other_config, "hw-offload", false) && > + !flow_restore_wait) { > static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > > if (ovsthread_once_start(&once)) { > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > index 8237a85..08b192c 100644 > --- a/lib/netdev-offload.h > +++ b/lib/netdev-offload.h > @@ -124,7 +124,8 @@ int netdev_get_hw_info(struct netdev *, int); > void netdev_set_hw_info(struct netdev *, int, int); > bool netdev_any_oor(void); > bool netdev_is_flow_api_enabled(void); > -void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); > +void netdev_set_flow_api_enabled(const struct smap *ovs_other_config, > + bool flow_restore_wait); > bool netdev_is_offload_rebalance_policy_enabled(void); > int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index e328d8e..bcbb176 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -813,7 +813,8 @@ datapath_reconfigure(const struct ovsrec_open_vswitch > *cfg) > } > > static void > -bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) > +bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg, > + bool flow_restore_wait) > { > struct sockaddr_in *managers; > struct bridge *br; > @@ -922,7 +923,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch > *ovs_cfg) > /* Clear eventual previous errors */ > ovsrec_interface_set_error(iface->cfg, NULL); > iface_configure_cfm(iface); > - iface_configure_qos(iface, port->cfg->qos); > + if (!flow_restore_wait) { > + iface_configure_qos(iface, port->cfg->qos); > + } Guess you disable qos because it might be using tc to do the ingress policing? However I do not think just disabling QoS, in general, will be wise, as we do not QoS with existing flows, don’t you think so? > iface_set_mac(br, port, iface); > ofproto_port_set_bfd(br->ofproto, iface->ofp_port, > &iface->cfg->bfd); > @@ -3289,7 +3292,8 @@ bridge_run(void) > cfg = ovsrec_open_vswitch_first(idl); > > if (cfg) { > - netdev_set_flow_api_enabled(&cfg->other_config); > + netdev_set_flow_api_enabled(&cfg->other_config, > + ofproto_get_flow_restore_wait()); > dpdk_init(&cfg->other_config); No sure how this will work, as the ofproto_get_flow_restore_wait() variable is not set until a couple of lines below: 3291 if (cfg) { 3292 netdev_set_flow_api_enabled(&cfg->other_config); 3293 dpdk_init(&cfg->other_config); 3294 userspace_tso_init(&cfg->other_config); 3295 } 3296 3297 /* Initialize the ofproto library. This only needs to run once, but 3298 * it must be done after the configuration is set. If the 3299 * initialization has already occurred, bridge_init_ofproto() 3300 * returns immediately. */ 3301 bridge_init_ofproto(cfg); 3302 3303 /* Once the value of flow-restore-wait is false, we no longer should 3304 * check its value from the database. */ 3305 if (cfg && ofproto_get_flow_restore_wait()) { 3306 ofproto_set_flow_restore_wait(smap_get_bool(&cfg->other_config, 3307 "flow-restore-wait", false)); 3308 } Also as you are disabling tc offload fully, you might as well do the check outside the netdev_set_flow_api_enabled(). Something like: if (!ofproto_get_flow_restore_wait()) { netdev_set_flow_api_enabled(&cfg->other_config); } However, the main problem with this patch might be the following statement in the documentation: “The default value is <code>false</code>. Changing this value requires restarting the daemon” I’ve dealt with a case in the past that was failing because I enabled offload but did not restart. Unfortunately, I can not remember the details, I think it had something to do with feature detection. I’ve copied on some more fooks who might know more details about this requirement. > userspace_tso_init(&cfg->other_config); > } > @@ -3328,7 +3332,8 @@ bridge_run(void) > > idl_seqno = ovsdb_idl_get_seqno(idl); > txn = ovsdb_idl_txn_create(idl); > - bridge_reconfigure(cfg ? cfg : &null_cfg); > + bridge_reconfigure(cfg ? cfg : &null_cfg, > + ofproto_get_flow_restore_wait()); > > if (cfg) { > ovsrec_open_vswitch_set_cur_cfg(cfg, cfg->next_cfg); > -- > 1.8.3.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
