On 15 Apr 2022, at 11:45, wenxu wrote:
> At 2022-04-15 17:15:06, "Eelco Chaudron" <[email protected]> wrote: >> 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? > Yes the ingress policing will del the ingress qdisc. What do you mean for "do > not QoS with existing flows", Just don't del the qdisc with > flow_restore_wait flag？ >> >>> 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: > Yes It is a problem Although I test success for the default value is true. >> >> 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” >> > > What do you mean for this? This patch used for vswitchd restart with > restore-flow-wait flag. If you enable hardware offload you need to restart vswitchd, so that it get initialized in the first bridge run. If you do no restart ovswitchd, i.e., enable it in the next bridge run, you could run into issues. Which is exactly what you are doing here. >> >>> 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
