Re: [ovs-dev] ovn: Handling of arp/nd learning on logical switches
Tangentially related to this: I was trying to detect when the problem with the "too many resubmits" occurs, for alerting purposes. Some light examination of the source of ovs suggested that the coverage counter "drop_action_too_many_resubmit" corresponds to the error "XLATE_TOO_MANY_RESUBMITS". So I saw this very recent log message in /var/log/openvswitch/ovs-vswitchd.log (I censored some addresses): 2023-03-03T10:06:51.453Z|00518|ofproto_dpif_xlate(handler34)|WARN|over 4096 resubmit actions on bridge br-int while processing arp,in_port=1,vlan_tci=0x,dl_src=zz:zz:zz:zz:zz:zz,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=xxx.xxx.xxx.xxx,arp_tpa=yyy.yyy.yyy.yyy,arp_op=1,arp_sha=vv:vv:vv:vv:vv:vv,arp_tha=00:00:00:00:00:00 but the coverage counter seems to ignore this: # ovs-appctl coverage/read-counter drop_action_too_many_resubmit 0 Is this a bug or am I looking at the wrong thing(s)? Cheers, -Olaf. -- Olaf Seibert Site Reliability Engineer SysEleven GmbH Boxhagener Straße 80 10245 Berlin T +49 30 233 2012 0 F +49 30 616 7555 0 https://www.syseleven.de https://www.linkedin.com/company/syseleven-gmbh/ https://www.twitter.com/SysEleven https://www.syseleven.de/events/ Aktueller System-Status immer unter: https://www.syseleven-status.net/ Firmensitz: Berlin Registergericht: AG Berlin Charlottenburg, HRB 108571 Berlin Geschäftsführer: Marc Korthaus, Jens Ihlenfeld, Andreas Hermann, Norbert Müller ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: Fix memory leak.
Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 cloned the external_ids to ids, then passed them to sbrec_port_binding_set_external_ids(). The intermediate step is unneeded (and caused a memory leak). Pass the external_ids directly. Signed-off-by: Olaf Seibert --- northd/northd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..309b449dd 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3435,9 +3435,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, sbrec_port_binding_set_mac(op->sb, , 1); ds_destroy(); -struct smap ids = SMAP_INITIALIZER(); -smap_clone(, >nbrp->external_ids); -sbrec_port_binding_set_external_ids(op->sb, ); +sbrec_port_binding_set_external_ids(op->sb, >nbrp->external_ids); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ovn] Memory leak in ovn-northd: which fix is preferred?
Hi! We discovered a memory leak in ovn-northd, which grows quickly, caused by commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 (dated Wed Feb 23 09:30:00 2022 +). We also found a fix for it. I am however not familiar with this code, so when I looked a bit further, I wondered if a different variant would be better. So here are three variants for consideration. Here is the first variant which is the simplest change, and the one we actually tested: >From 491008b30fd2e7e09cc4cd3aab1d1a0a3f86dca7 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:15:47 +0200 Subject: [PATCH ovn] Fix memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 populated ids with names, but didn't clean them up. Do that here. Signed-off-by: Olaf Seibert --- northd/northd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..e7beed8fb 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3438,6 +3438,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, struct smap ids = SMAP_INITIALIZER(); smap_clone(, >nbrp->external_ids); sbrec_port_binding_set_external_ids(op->sb, ); +smap_destroy(); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 I realised that the `ids` variable could possibly be removed, since these external_ids are in effect just passed on unmodified to sbrec_port_binding_set_external_ids(). But since I didn't find the source to that, I am not totally sure that this is safe to do. So variant 2 uses that shortcut: >From 6472d24678d3af1d622bacf7da1c9ba77e058b10 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:17:12 +0200 Subject: [PATCH ovn] Fix memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 cloned the external_ids to ids, then passed them to sbrec_port_binding_set_external_ids(). The intermediate step is unneeded (and caused a memory leak). Pass the external_ids directly. Signed-off-by: Olaf Seibert --- northd/northd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..309b449dd 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3435,9 +3435,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, sbrec_port_binding_set_mac(op->sb, , 1); ds_destroy(); -struct smap ids = SMAP_INITIALIZER(); -smap_clone(, >nbrp->external_ids); -sbrec_port_binding_set_external_ids(op->sb, ); +sbrec_port_binding_set_external_ids(op->sb, >nbrp->external_ids); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 Then, further down in northd.c, there is another use of sbrec_port_binding_set_nat_addresses(). But in that location, there is a manipulation of the names. Maybe the same is needed here. On the other hand, there the source of the ids is op->nbsp instead of op->nbrp here. So I'm not sure if it has to be the same. Variant 3: >From 4a61a02009eb450c6dfadd156968023833e4f1a6 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:33:23 +0200 Subject: [PATCH ovn] Fix a memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 added another place where external_ids are copied to the SB database. This had a memory leak, and also differed from the other place where this happens. Make this case the same as the other one. Signed-off-by: Olaf Seibert --- northd/northd.c | 5 + 1 file changed, 5 insertions(+) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..4d34f7ebc 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3437,7 +3437,12 @@ ovn_port_update_sbrec(struct northd_input *input_data, struct smap ids = SMAP_INITIALIZER(); smap_clone(, >nbrp->external_ids); +const char *name = smap_get(, "neutron:port_name"); +if (name && name[0]) { +smap_add(, "name", name); +} sbrec_port_binding_set_external_ids(op->sb, ); +smap_destroy(); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 So which variant would be preferred? It looks like variant #3 might be needed, due to the "name" name. If not, #2 would be the shortest version. If that one is inappropriate, variant #1 would be it. Cheers, -Olaf Seibert ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ovn] Memory leak in ovn-northd: which fix is preferred?
Hi! We discovered a memory leak in ovn-northd, which grows quickly, caused by commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 (dated Wed Feb 23 09:30:00 2022 +). We also found a fix for it. I am however not familiar with this code, so when I looked a bit further, I wondered if a different variant would be better. So here are three variants for consideration. Here is the first variant which is the simplest change, and the one we actually tested: >From 491008b30fd2e7e09cc4cd3aab1d1a0a3f86dca7 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:15:47 +0200 Subject: [PATCH ovn] Fix memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 populated ids with names, but didn't clean them up. Do that here. Signed-off-by: Olaf Seibert --- northd/northd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..e7beed8fb 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3438,6 +3438,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, struct smap ids = SMAP_INITIALIZER(); smap_clone(, >nbrp->external_ids); sbrec_port_binding_set_external_ids(op->sb, ); +smap_destroy(); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 I realised that the `ids` variable could possibly be removed, since these external_ids are in effect just passed on unmodified to sbrec_port_binding_set_external_ids(). But since I didn't find the source to that, I am not totally sure that this is safe to do. So variant 2 uses that shortcut: >From 6472d24678d3af1d622bacf7da1c9ba77e058b10 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:17:12 +0200 Subject: [PATCH ovn] Fix memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 cloned the external_ids to ids, then passed them to sbrec_port_binding_set_external_ids(). The intermediate step is unneeded (and caused a memory leak). Pass the external_ids directly. Signed-off-by: Olaf Seibert --- northd/northd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..309b449dd 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3435,9 +3435,7 @@ ovn_port_update_sbrec(struct northd_input *input_data, sbrec_port_binding_set_mac(op->sb, , 1); ds_destroy(); -struct smap ids = SMAP_INITIALIZER(); -smap_clone(, >nbrp->external_ids); -sbrec_port_binding_set_external_ids(op->sb, ); +sbrec_port_binding_set_external_ids(op->sb, >nbrp->external_ids); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 Then, further down in northd.c, there is another use of sbrec_port_binding_set_nat_addresses(). But in that location, there is a manipulation of the names. Maybe the same is needed here. On the other hand, there the source of the ids is op->nbsp instead of op->nbrp here. So I'm not sure if it has to be the same. Variant 3: >From 4a61a02009eb450c6dfadd156968023833e4f1a6 Mon Sep 17 00:00:00 2001 From: Olaf Seibert Date: Fri, 12 Aug 2022 11:33:23 +0200 Subject: [PATCH ovn] Fix a memory leak. Commit 7b56f69580e1f390d9c6753a2cb8f0dbfbb4c467 added another place where external_ids are copied to the SB database. This had a memory leak, and also differed from the other place where this happens. Make this case the same as the other one. Signed-off-by: Olaf Seibert --- northd/northd.c | 5 + 1 file changed, 5 insertions(+) diff --git a/northd/northd.c b/northd/northd.c index 09eccf4d9..4d34f7ebc 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3437,7 +3437,12 @@ ovn_port_update_sbrec(struct northd_input *input_data, struct smap ids = SMAP_INITIALIZER(); smap_clone(, >nbrp->external_ids); +const char *name = smap_get(, "neutron:port_name"); +if (name && name[0]) { +smap_add(, "name", name); +} sbrec_port_binding_set_external_ids(op->sb, ); +smap_destroy(); sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } else { -- 2.37.2 So which variant would be preferred? It looks like variant #3 might be needed, due to the "name" name. If not, #2 would be the shortest version. If that one is inappropriate, variant #1 would be it. Cheers, -Olaf Seibert ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev