| Message ID | 1649814049-108122-1-git-send-email-wenx05124561@163.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-offload: make netdev-offload work with flow-restore-wait in tc mode | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hi Wenxu, First FYI you send your emails from wenx05124561@163.com but the from in the header has wenxu@chinatelecom.cn. 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 <wenxu@chinatelecom.cn> > > 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 <wenxu@chinatelecom.cn> > --- > 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 >
At 2022-04-15 17:15:06, "Eelco Chaudron" <echaudro@redhat.com> wrote: >Hi Wenxu, > >First FYI you send your emails from wenx05124561@163.com but the from in the header has wenxu@chinatelecom.cn. 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 <wenxu@chinatelecom.cn> >> >> 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 <wenxu@chinatelecom.cn> >> --- >> 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. > >> 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 >>
On 15 Apr 2022, at 11:45, wenxu wrote: > At 2022-04-15 17:15:06, "Eelco Chaudron" <echaudro@redhat.com> wrote: >> Hi Wenxu, >> >> First FYI you send your emails from wenx05124561@163.com but the from in the header has wenxu@chinatelecom.cn. 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 <wenxu@chinatelecom.cn> >>> >>> 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 <wenxu@chinatelecom.cn> >>> --- >>> 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 >>>
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); + } 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); 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);