On Fri, Apr 22, 2022 at 1:41 AM Eelco Chaudron <[email protected]> wrote:
>
>
>
> 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]>
Hi, I found this patch useful, but it seems inactive for a long time. I
hope we can revive and update it.
Regardless of the issues pointed out by Eelco, this patch works well for
traffic not going through tunnels, but for tunnelled traffic, e.g. geneve
traffic, I found that even with the patch, when OVS starts, the ingress
qdisc for the genev_sys_6081 device is deleted, so the traffic is still
broken even with flow-restore-wait set. I didn't find yet in the code where
it could be deleted. Any hint/insight would be appreciated.
> > ---
> > 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?
>
Agree. This is incorrect. I think it is better to use the approach from v1,
to avoid configuring qos when flow-restore-wait=true, hopefully good enough
for the short period before flow restore is done.
> > +
> > 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.
>
>
Based on my test, this seems to be fine, but I am not sure if I am just
lucky with my environment.
Thanks,
Han
>
> > + }
> > 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
