diff mbox series

[ovs-dev,2/2] ofproto-dpif: avoid unneccesary backer revalidation

Message ID 8c82ba08f6ecd59a82866267f300c34ec1fe78db.1649149317.git.lic121@chinatelecom.cn
State Changes Requested
Headers show
Series fix revalidation triggers | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Cheng Li April 5, 2022, 10:16 a.m. UTC 
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.

This patch also fix lldp memory leak bug:
lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
is supposed to free the memory, but it doesn't.

Signed-off-by: lic121 <lic121@chinatelecom.cn>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/lldp/lldpd.c       | 10 +++-------
 lib/ovs-lldp.c         |  8 ++++++++
 lib/ovs-lldp.h         |  1 +
 ofproto/ofproto-dpif.c |  7 +++++--
 tests/ofproto-dpif.at  | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 9 deletions(-)

Comments

Eelco Chaudron April 8, 2022, 7:45 a.m. UTC | #1 
On 5 Apr 2022, at 12:16, lic121 wrote:

> If lldp didn't change, we are not supposed to trigger backer
> revalidation.
> Without this patch, bridge_reconfigure() always trigger udpif
> revalidator because of lldp.
>
> This patch also fix lldp memory leak bug:
> lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
> is supposed to free the memory, but it doesn't.
>
> Signed-off-by: lic121 <lic121@chinatelecom.cn>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
> ---

Changes look good me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Ilya Maximets May 26, 2022, 9:51 a.m. UTC | #2 
On 4/5/22 12:16, lic121 wrote:
> If lldp didn't change, we are not supposed to trigger backer
> revalidation.
> Without this patch, bridge_reconfigure() always trigger udpif
> revalidator because of lldp.
> 
> This patch also fix lldp memory leak bug:
> lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
> is supposed to free the memory, but it doesn't.

It looks like these are two separate changes.
Could you, please, split the patch in two?

Best regards, Ilya Maximets.
Cheng Li May 26, 2022, 1:45 p.m. UTC | #3 
On Thu, May 26, 2022 at 11:51:31AM +0200, Ilya Maximets wrote:
> On 4/5/22 12:16, lic121 wrote:
> > If lldp didn't change, we are not supposed to trigger backer
> > revalidation.
> > Without this patch, bridge_reconfigure() always trigger udpif
> > revalidator because of lldp.
> > 
> > This patch also fix lldp memory leak bug:
> > lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
> > is supposed to free the memory, but it doesn't.
> 
> It looks like these are two separate changes.
> Could you, please, split the patch in two?

Sure, will do that.

> 
> Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index 403f1f5..4bff7b0 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -140,13 +140,9 @@  lldpd_cleanup(struct lldpd *cfg)
     VLOG_DBG("cleanup all ports");
 
     LIST_FOR_EACH_SAFE (hw, h_entries, &cfg->g_hardware) {
-        if (!hw->h_flags) {
-            ovs_list_remove(&hw->h_entries);
-            lldpd_remote_cleanup(hw, NULL, true);
-            lldpd_hardware_cleanup(cfg, hw);
-        } else {
-            lldpd_remote_cleanup(hw, NULL, false);
-        }
+        ovs_list_remove(&hw->h_entries);
+        lldpd_remote_cleanup(hw, NULL, true);
+        lldpd_hardware_cleanup(cfg, hw);
     }
 
     VLOG_DBG("cleanup all chassis");
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index a9d205e..2d13e97 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@  lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
     ovs_mutex_unlock(&mutex);
 }
 
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+    return lldp ? lldp->enabled : false;
+}
+
 /* Configures the LLDP stack.
  */
 bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@  void lldp_run(struct lldpd *cfg);
 bool lldp_should_send_packet(struct lldp *cfg);
 bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
 bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
 void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
 void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
                      const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7a1bb0d..3ae44f0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2490,11 +2490,11 @@  set_lldp(struct ofport *ofport_,
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+    bool old_enable = lldp_is_enabled(ofport->lldp);
     int error = 0;
 
-    if (cfg) {
+    if (cfg && !smap_is_empty(cfg)) {
         if (!ofport->lldp) {
-            ofproto->backer->need_revalidate = REV_RECONFIGURE;
             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
         }
 
@@ -2506,6 +2506,9 @@  set_lldp(struct ofport *ofport_,
     } else if (ofport->lldp) {
         lldp_unref(ofport->lldp);
         ofport->lldp = NULL;
+    }
+
+    if (lldp_is_enabled(ofport->lldp) != old_enable) {
         ofproto->backer->need_revalidate = REV_RECONFIGURE;
     }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index dbb3b6d..1c9a94c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@  AT_CHECK([ovs-appctl revalidator/wait])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+    [add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
 
 dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and