| Message ID | 1554827868-15231-1-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev,v3] netdev-linux: Don't remove ingress when not configured | expand |
Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (lib/netdev-linux.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 netdev-linux: Don't remove ingress when not configured The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
should apply the http://patchwork.ozlabs.org/patch/1081918/ and then test this patch On Wed, Apr 10, 2019 at 2:12 AM 0-day Robot <robot@bytheb.org> wrote: > > Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > fatal: sha1 information is lacking or useless (lib/netdev-linux.c). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 netdev-linux: Don't remove ingress when not configured > The copy of the patch that failed is found in: > /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch > When you have resolved this problem, run "git am --resolved". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > Please check this out. If you feel there has been an error, please email aconole@bytheb.org > > Thanks, > 0-day Robot
On Tue, Apr 09, 2019 at 12:37:48PM -0400, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > In some case, we may not use the openvswitch tc to limit the ingress > police rate. And before we add the port to openvswitch bridge, we may > set the ingress policer, so don't remove the ingress when we not configured > it in openvswitch. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > lib/netdev-linux.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 0fce217..ad8a244 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2448,6 +2448,7 @@ netdev_linux_set_policing(struct netdev *netdev_, > const char *netdev_name = netdev_get_name(netdev_); > int ifindex; > int error; > + bool should_cleanup_ingress = false; > > kbits_burst = (!kbits_rate ? 0 /* Force to 0 if no rate specified. */ > : !kbits_burst ? 8000 /* Default to 8000 kbits if 0. */ > @@ -2466,14 +2467,10 @@ netdev_linux_set_policing(struct netdev *netdev_, > /* Assume that settings haven't changed since we last set them. */ > goto out; > } > + should_cleanup_ingress = true; > netdev->cache_valid &= ~VALID_POLICING; > } > > - error = get_ifindex(netdev_, &ifindex); > - if (error) { > - goto out; > - } > - > COVERAGE_INC(netdev_set_policing); > > /* Use matchall for policing when offloadling ovs with tc-flower. */ Thanks for working on this. This new version uses the logic that if and only if there's a cached policing setting, then OVS must have set it up, and only in that case should OVS clean it up. But the assumption is wrong. The setting cache is just a cache and it gets reset if the kernel notifies us of a change to a device. It also doesn't persist from one OVS run to another, so this wouldn't allow OVS to turn off policing if it set it and then segfaulted and restarted (or the admin stopped and started it).
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 0fce217..ad8a244 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2448,6 +2448,7 @@ netdev_linux_set_policing(struct netdev *netdev_, const char *netdev_name = netdev_get_name(netdev_); int ifindex; int error; + bool should_cleanup_ingress = false; kbits_burst = (!kbits_rate ? 0 /* Force to 0 if no rate specified. */ : !kbits_burst ? 8000 /* Default to 8000 kbits if 0. */ @@ -2466,14 +2467,10 @@ netdev_linux_set_policing(struct netdev *netdev_, /* Assume that settings haven't changed since we last set them. */ goto out; } + should_cleanup_ingress = true; netdev->cache_valid &= ~VALID_POLICING; } - error = get_ifindex(netdev_, &ifindex); - if (error) { - goto out; - } - COVERAGE_INC(netdev_set_policing); /* Use matchall for policing when offloadling ovs with tc-flower. */ @@ -2486,14 +2483,22 @@ netdev_linux_set_policing(struct netdev *netdev_, return error; } - /* Remove any existing ingress qdisc. */ - error = tc_add_del_ingress_qdisc(ifindex, false, 0); + error = get_ifindex(netdev_, &ifindex); if (error) { - VLOG_WARN_RL(&rl, "%s: removing policing failed: %s", - netdev_name, ovs_strerror(error)); goto out; } + /* Should consider the kbits_rate/kbits_burst changed and + * the case that kbits_rate has been set in db. */ + if (should_cleanup_ingress || kbits_rate) { + error = tc_add_del_ingress_qdisc(ifindex, false, 0); + if (error) { + VLOG_WARN_RL(&rl, "%s: removing policing failed: %s", + netdev_name, ovs_strerror(error)); + goto out; + } + } + if (kbits_rate) { error = tc_add_del_ingress_qdisc(ifindex, true, 0); if (error) {