On 15 Apr 2022, at 13:25, [email protected] wrote:
> 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-linux.c | 16 ++++++++-------- > vswitchd/bridge.c | 18 ++++++++++-------- > 2 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 9d12502..b6315e3 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2784,17 +2784,17 @@ netdev_linux_set_policing(struct netdev *netdev_, > uint32_t kbits_rate, > goto out; > } > > - /* Remove any existing ingress qdisc. */ > - error = tc_add_del_qdisc(ifindex, false, 0, TC_INGRESS); > - if (error) { > - VLOG_WARN_RL(&rl, "%s: removing policing failed: %s", > - netdev_name, ovs_strerror(error)); > - goto out; > - } > - > if (kbits_rate || kpkts_rate) { > const char *cls_name = "matchall"; > > + /* Remove any existing ingress qdisc. */ > + error = tc_add_del_qdisc(ifindex, false, 0, TC_INGRESS); > + if (error) { > + VLOG_WARN_RL(&rl, "%s: removing policing failed: %s", > + netdev_name, ovs_strerror(error)); > + goto out; > + } Are we sure we are not breaking a corner case here where something might already have been configured, and we are not cleaning it up? I.e. what if we configured some rate limiting, and now we unconfigure it, the values are all zero, and it will not be removed? > + > error = tc_add_del_qdisc(ifindex, true, 0, TC_INGRESS); > if (error) { > VLOG_WARN_RL(&rl, "%s: adding policing qdisc failed: %s", > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index e328d8e..d8843a7 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -3288,8 +3288,17 @@ bridge_run(void) > } > cfg = ovsrec_open_vswitch_first(idl); > > + /* Once the value of flow-restore-wait is false, we no longer should > + * check its value from the database. */ > + if (cfg && ofproto_get_flow_restore_wait()) { > + ofproto_set_flow_restore_wait(smap_get_bool(&cfg->other_config, > + "flow-restore-wait", false)); > + } > + > if (cfg) { > - netdev_set_flow_api_enabled(&cfg->other_config); > + if (!ofproto_get_flow_restore_wait()) { > + netdev_set_flow_api_enabled(&cfg->other_config); See my previous comment here, this needs more input: 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 folks who might know more details about this requirement. > + } > dpdk_init(&cfg->other_config); > userspace_tso_init(&cfg->other_config); > } > @@ -3300,13 +3309,6 @@ bridge_run(void) > * returns immediately. */ > bridge_init_ofproto(cfg); > > - /* Once the value of flow-restore-wait is false, we no longer should > - * check its value from the database. */ > - if (cfg && ofproto_get_flow_restore_wait()) { > - ofproto_set_flow_restore_wait(smap_get_bool(&cfg->other_config, > - "flow-restore-wait", false)); > - } > - > bridge_run__(); > > /* Re-configure SSL. We do this on every trip through the main loop, _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
