[ovs-dev] [PATCH ovn v2 3/3] treewide: remove next variable in _SAFE loops
Remove the use of the next variable when possible in _SAFE loops.
This makes code more readable and less error prone.
Signed-off-by: Adrian Moreno
---
controller-vtep/binding.c | 4 +-
controller-vtep/gateway.c | 4 +-
controller-vtep/vtep.c | 4 +-
controller/binding.c| 21 +++---
controller/encaps.c | 4 +-
controller/if-status.c | 17 ++---
controller/lflow-cache.c| 3 +-
controller/lflow-conj-ids.c | 18 +++--
controller/lflow.c | 36 +-
controller/ofctrl-seqno.c | 4 +-
controller/ofctrl.c | 77 ++--
controller/ovn-controller.c | 20 +++---
controller/patch.c | 4 +-
controller/physical.c | 4 +-
controller/pinctrl.c| 69 +-
controller/vif-plug.c | 8 +--
ic/ovn-ic.c | 12 ++--
lib/expr.c | 20 +++---
lib/extend-table.c | 20 +++---
lib/extend-table.h | 4 +-
lib/vif-plug-provider.c | 8 +--
northd/northd.c | 139 +---
northd/ovn-northd-ddlog.c | 4 +-
northd/ovn-northd.c | 16 ++---
utilities/ovn-ic-nbctl.c| 4 +-
utilities/ovn-ic-sbctl.c| 4 +-
utilities/ovn-nbctl.c | 8 +--
utilities/ovn-sbctl.c | 4 +-
utilities/ovn-trace.c | 4 +-
29 files changed, 259 insertions(+), 285 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index 1ee52b592..0d0c1ab16 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -207,8 +207,8 @@ binding_run(struct controller_vtep_ctx *ctx)
}
}
-struct shash_node *iter, *next;
-SHASH_FOR_EACH_SAFE (iter, next, _map) {
+struct shash_node *iter;
+SHASH_FOR_EACH_SAFE (iter, _map) {
struct ps *ps = iter->data;
struct shash_node *node;
diff --git a/controller-vtep/gateway.c b/controller-vtep/gateway.c
index 9fbfc0337..2a8714e9a 100644
--- a/controller-vtep/gateway.c
+++ b/controller-vtep/gateway.c
@@ -135,11 +135,11 @@ revalidate_gateway(struct controller_vtep_ctx *ctx)
simap_put(_chassis_map, pswitch->name, gw_reval_seq);
}
-struct simap_node *iter, *next;
+struct simap_node *iter;
/* For 'gw_node' in 'gw_chassis_map' whose data is not
* 'gw_reval_seq', it means the corresponding physical switch no
* longer exist. So, garbage collects them. */
-SIMAP_FOR_EACH_SAFE (iter, next, _chassis_map) {
+SIMAP_FOR_EACH_SAFE (iter, _chassis_map) {
if (iter->data != gw_reval_seq) {
const struct sbrec_chassis *chassis_rec;
diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
index ecca00b54..784a7e735 100644
--- a/controller-vtep/vtep.c
+++ b/controller-vtep/vtep.c
@@ -426,8 +426,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct
shash *ucast_macs_rmts,
SHASH_FOR_EACH (node, ucast_macs_rmts) {
vteprec_ucast_macs_remote_delete(node->data);
}
-struct ls_hash_node *iter, *next;
-HMAP_FOR_EACH_SAFE (iter, next, hmap_node, _map) {
+struct ls_hash_node *iter;
+HMAP_FOR_EACH_SAFE (iter, hmap_node, _map) {
struct vtep_rec_physical_locator_list_entry *ploc_entry;
vtep_update_mmr(vtep_idl_txn, >locators_list,
iter->vtep_ls, iter->mmr_ext);
diff --git a/controller/binding.c b/controller/binding.c
index 1259e6b3b..7281b0485 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -607,15 +607,15 @@ local_binding_data_init(struct local_binding_data
*lbinding_data)
void
local_binding_data_destroy(struct local_binding_data *lbinding_data)
{
-struct shash_node *node, *next;
+struct shash_node *node;
-SHASH_FOR_EACH_SAFE (node, next, _data->lports) {
+SHASH_FOR_EACH_SAFE (node, _data->lports) {
struct binding_lport *b_lport = node->data;
binding_lport_destroy(b_lport);
shash_delete(_data->lports, node);
}
-SHASH_FOR_EACH_SAFE (node, next, _data->bindings) {
+SHASH_FOR_EACH_SAFE (node, _data->bindings) {
struct local_binding *lbinding = node->data;
local_binding_destroy(lbinding, _data->lports);
shash_delete(_data->bindings, node);
@@ -2295,8 +2295,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in
*b_ctx_in,
}
struct shash_node *node;
-struct shash_node *node_next;
-SHASH_FOR_EACH_SAFE (node, node_next, _container_pbs) {
+SHASH_FOR_EACH_SAFE (node, _container_pbs) {
handled = handle_deleted_vif_lport(node->data, LP_CONTAINER, b_ctx_in,
b_ctx_out);
shash_delete(_container_pbs, node);
@@ -2305,7 +2304,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in
*b_ctx_in,
}
}
-SHASH_FOR_EACH_SAFE (node, node_next, _virtual_pbs) {
+SHASH_FOR_EACH_SAFE (node, _virtual_pbs) {
handled =
[ovs-dev] [PATCH ovn v2 2/3] parallel-hmap: rewrite iterator using multivar helpers
Rewrite the parallel hmap iterator macros using multivariable iterator
helpers to avoid undefined behavior.
Suggested-by: Dumitru Ceara
Signed-off-by: Adrian Moreno
---
lib/ovn-parallel-hmap.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/ovn-parallel-hmap.h b/lib/ovn-parallel-hmap.h
index 897208ef8..0f7d68770 100644
--- a/lib/ovn-parallel-hmap.h
+++ b/lib/ovn-parallel-hmap.h
@@ -58,11 +58,11 @@ extern "C" {
* ThreadID + step * i as the JOBID parameter.
*/
-#define HMAP_FOR_EACH_IN_PARALLEL(NODE, MEMBER, JOBID, HMAP) \
- for (INIT_CONTAINER(NODE, hmap_first_in_bucket_num(HMAP, JOBID), MEMBER); \
-(NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) \
- || ((NODE = NULL), false); \
- ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_IN_PARALLEL(NODE, MEMBER, JOBID, HMAP)\
+ for (INIT_MULTIVAR(NODE, MEMBER, hmap_first_in_bucket_num(HMAP, JOBID), \
+ struct hmap_node);\
+CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL); \
+UPDATE_MULTIVAR(NODE, hmap_next_in_bucket(ITER_VAR(NODE
/* We do not have a SAFE version of the macro, because the hash size is not
* atomic and hash removal operations would need to be wrapped with
--
2.35.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 0/3] Use newest OVS version to fix Undefined Behavior
A series has been recently introduced in OVS that has two main benefits: - Undefined Behavior in iterator loops is removed. This enforces some restrictions on how this macros can be used, namely: * User-provided iterator variable is set to NULL after the loop ends normally * In _SAFE version of the loop, it's not always safe to use the 'next' variable. When it would point to the list's head, it is instead set to NULL by the iteration macros. - _SAFE version of the iterator macros now do not require the user to provide a 'next' variable leading to cleaner and less error prone code. This series bumps ovs to the latest HEAD in master branch and adapts the code accordingly. v1->v2: - Rebase - Use ovs branch-2.17 Adrian Moreno (3): treewide: bump ovs and fix problematic loops parallel-hmap: rewrite iterator using multivar helpers treewide: remove next variable in _SAFE loops controller-vtep/binding.c | 4 +- controller-vtep/gateway.c | 4 +- controller-vtep/vtep.c | 4 +- controller/binding.c| 21 +++--- controller/encaps.c | 4 +- controller/if-status.c | 17 ++--- controller/lflow-cache.c| 3 +- controller/lflow-conj-ids.c | 18 +++-- controller/lflow.c | 36 +- controller/ofctrl-seqno.c | 4 +- controller/ofctrl.c | 84 +++--- controller/ovn-controller.c | 20 +++--- controller/patch.c | 4 +- controller/physical.c | 4 +- controller/pinctrl.c| 69 +- controller/vif-plug.c | 8 +-- ic/ovn-ic.c | 12 ++-- lib/actions.c | 2 +- lib/expr.c | 51 +++-- lib/extend-table.c | 20 +++--- lib/extend-table.h | 4 +- lib/ovn-parallel-hmap.h | 10 +-- lib/ovn-util.c | 2 +- lib/vif-plug-provider.c | 8 +-- northd/northd.c | 139 +--- northd/ovn-northd-ddlog.c | 4 +- northd/ovn-northd.c | 16 ++--- ovs | 2 +- utilities/ovn-ic-nbctl.c| 4 +- utilities/ovn-ic-sbctl.c| 4 +- utilities/ovn-nbctl.c | 14 ++-- utilities/ovn-sbctl.c | 4 +- utilities/ovn-trace.c | 12 ++-- 33 files changed, 299 insertions(+), 313 deletions(-) -- 2.35.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 1/3] treewide: bump ovs and fix problematic loops
The minimum changes to adapt to new ovs are:
- adapt to changes made in inet_parse_active function by passing an
extra NULL argument
- fix those places where next variable was being used inside *_SAFE
iterators since. Now the next variable might not always be safe to use
so extra checks are needed.
Signed-off-by: Adrian Moreno
---
controller/ofctrl.c | 7 ++-
lib/actions.c | 2 +-
lib/expr.c| 31 ++-
lib/ovn-util.c| 2 +-
ovs | 2 +-
utilities/ovn-nbctl.c | 6 +++---
utilities/ovn-trace.c | 8 +---
7 files changed, 35 insertions(+), 23 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index eb66d0348..3b9d71733 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -943,7 +943,12 @@ link_installed_to_desired(struct installed_flow *i, struct
desired_flow *d)
break;
}
}
-ovs_list_insert(>installed_ref_list_node, >installed_ref_list_node);
+if (!f) {
+ovs_list_insert(>desired_refs, >installed_ref_list_node);
+} else {
+ovs_list_insert(>installed_ref_list_node,
+>installed_ref_list_node);
+}
d->installed_flow = i;
return installed_flow_get_active(i) == d;
}
diff --git a/lib/actions.c b/lib/actions.c
index 7fe80f458..a3cf747db 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2440,7 +2440,7 @@ validate_empty_lb_backends(struct action_context *ctx,
switch (o->option->code) {
case EMPTY_LB_VIP:
-if (!inet_parse_active(c->string, 0, , false)) {
+if (!inet_parse_active(c->string, 0, , false, NULL)) {
lexer_error(ctx->lexer, "Invalid load balancer VIP '%s'",
c->string);
return;
diff --git a/lib/expr.c b/lib/expr.c
index ca333f3d2..b6374a310 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -198,16 +198,17 @@ expr_combine(enum expr_type type, struct expr *a, struct
expr *b)
}
static void
-expr_insert_andor(struct expr *andor, struct expr *before, struct expr *new)
+expr_insert_andor(struct expr *andor, struct ovs_list *before,
+ struct expr *new)
{
if (new->type == andor->type) {
if (andor->type == EXPR_T_AND) {
/* Conjunction junction, what's your function? */
}
-ovs_list_splice(>node, new->andor.next, >andor);
+ovs_list_splice(before, new->andor.next, >andor);
expr_destroy(new);
} else {
-ovs_list_insert(>node, >node);
+ovs_list_insert(before, >node);
}
}
@@ -2088,7 +2089,8 @@ expr_annotate__(struct expr *expr, const struct shash
*symtab,
expr_destroy(expr);
return NULL;
}
-expr_insert_andor(expr, next, new_sub);
+expr_insert_andor(expr, next ? >node : >andor,
+ new_sub);
}
*errorp = NULL;
return expr;
@@ -2288,7 +2290,7 @@ expr_evaluate_condition(struct expr *expr,
struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
c_aux);
e = expr_fix(e);
-expr_insert_andor(expr, next, e);
+expr_insert_andor(expr, next ? >node : >andor, e);
}
return expr_fix(expr);
@@ -2321,7 +2323,8 @@ expr_simplify(struct expr *expr)
case EXPR_T_OR:
LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
ovs_list_remove(>node);
-expr_insert_andor(expr, next, expr_simplify(sub));
+expr_insert_andor(expr, next ? >node : >andor,
+ expr_simplify(sub));
}
return expr_fix(expr);
@@ -2431,12 +2434,13 @@ crush_and_string(struct expr *expr, const struct
expr_symbol *symbol)
* EXPR_T_OR with EXPR_T_CMP subexpressions. */
struct expr *sub, *next = NULL;
LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
+struct ovs_list *next_list = next ? >node : >andor;
ovs_list_remove(>node);
struct expr *new = crush_cmps(sub, symbol);
switch (new->type) {
case EXPR_T_CMP:
if (!singleton) {
-ovs_list_insert(>node, >node);
+ovs_list_insert(next_list, >node);
singleton = new;
} else {
bool match = !strcmp(new->cmp.string, singleton->cmp.string);
@@ -2450,7 +2454,7 @@ crush_and_string(struct expr *expr, const struct
expr_symbol *symbol)
case EXPR_T_AND:
OVS_NOT_REACHED();
case EXPR_T_OR:
-ovs_list_insert(>node, >node);
+ovs_list_insert(next_list, >node);
break;
case EXPR_T_BOOLEAN:
if (!new->boolean) {
@@ -2546,7 +2550,7 @@ crush_and_numeric(struct expr *expr, const struct
expr_symbol *symbol)
case EXPR_T_AND:
OVS_NOT_REACHED();
Re: [ovs-dev] [PATCH ovn 0/3] Use newest OVS version to fix Undefined Behavior
On 4/19/22 15:35, Mark Michelson wrote: On 4/19/22 05:06, Adrian Moreno wrote: On 4/8/22 21:03, Mark Michelson wrote: Hi Adrian, The patches look correct to me, however when I tried to apply them locally, I hit a failure: Applying: treewide: remove next variable in _SAFE loops error: sha1 information is lacking or useless (controller/ofctrl.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0003 treewide: remove next variable in _SAFE loops When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort" I'm pretty sure this just means it needs a rebase. If you can rebase and re-upload, that would be great. Sure, I'll rebase and post v2. One question: Patches 1 and 2 are the minimum required to fix the undefined behavior and update to the newer OVS source. Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. Should I send another patch updating LTS branches with only patches 1 and 2 and the correspondent bump in ovs version? The only reason to do that is if the patches you provide for the main branch do not apply cleanly when applied to branch-22.03. Currently branch-22.03 is the only LTS OVN branch, and it is the most recent version of OVN, so hopefully the patches you create for main will apply cleanly. Understood, thanks. On 4/6/22 10:10, Adrian Moreno wrote: A series has been recently introduced in OVS that has two main benefits: - Undefined Behavior in iterator loops is removed. This enforces some restrictions on how this macros can be used, namely: * User-provided iterator variable is set to NULL after the loop ends normally * In _SAFE version of the loop, it's not always safe to use the 'next' variable. When it would point to the list's head, it is instead set to NULL by the iteration macros. - _SAFE version of the iterator macros now do not require the user to provide a 'next' variable leading to cleaner and less error prone code. This series bumps ovs to the latest HEAD in master branch and adapts the code accordingly. Adrian Moreno (3): treewide: bump ovs and fix problematic loops parallel-hmap: rewrite iterator using multivar helpers treewide: remove next variable in _SAFE loops controller-vtep/binding.c | 4 +- controller-vtep/gateway.c | 4 +- controller-vtep/vtep.c | 4 +- controller/binding.c | 21 +++--- controller/encaps.c | 4 +- controller/if-status.c | 17 ++--- controller/lflow-cache.c | 3 +- controller/lflow-conj-ids.c | 18 +++-- controller/lflow.c | 36 +- controller/ofctrl-seqno.c | 4 +- controller/ofctrl.c | 84 +++--- controller/ovn-controller.c | 20 +++--- controller/patch.c | 4 +- controller/physical.c | 4 +- controller/pinctrl.c | 69 +- controller/vif-plug.c | 8 +-- ic/ovn-ic.c | 12 ++-- lib/actions.c | 2 +- lib/expr.c | 51 +++-- lib/extend-table.c | 20 +++--- lib/extend-table.h | 4 +- lib/ovn-parallel-hmap.h | 10 +-- lib/ovn-util.c | 2 +- lib/vif-plug-provider.c | 8 +-- northd/northd.c | 139 +--- northd/ovn-northd-ddlog.c | 4 +- northd/ovn-northd.c | 16 ++--- ovs | 2 +- utilities/ovn-ic-nbctl.c | 4 +- utilities/ovn-ic-sbctl.c | 4 +- utilities/ovn-nbctl.c | 14 ++-- utilities/ovn-sbctl.c | 4 +- utilities/ovn-trace.c | 12 ++-- 33 files changed, 299 insertions(+), 313 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] pinctrl.c: Send GARP only on chassis atached to l3gw
Thanks for the patch. Two comments for the test case below.
On Tue, Apr 19, 2022 at 1:12 AM Ales Musil wrote:
>
> The GARP was sent even on chassis that were not serving
> as l3gw for the specified router. This could lead to race
> on the physical network when multiple chassis send the same
> ARP but the physical interfaces have different port numbers
> on the external bridge.
>
> Add check to filter out port_bindings for l3gw that
> are sitting on different chassis.
>
> Reported-at: https://bugzilla.redhat.com/2062580
> Signed-off-by: Ales Musil
> ---
> controller/pinctrl.c | 2 +-
> tests/ovn.at | 106 +++
> 2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 25b37ee88..42d1c2a46 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -5554,7 +5554,7 @@ get_localnet_vifs_l3gwports(
> sbrec_port_binding_index_set_datapath(target, ld->datapath);
> SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
> sbrec_port_binding_by_datapath) {
> -if (!strcmp(pb->type, "l3gateway")
> +if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis)
> || !strcmp(pb->type, "patch")) {
> sset_add(local_l3gw_ports, pb->logical_port);
> }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f9551b843..a02a9dab2 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8707,6 +8707,112 @@ OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([send gratuitous arp for l3gateway only on selected chassis])
> +ovn_start
> +
> +# Create logical switch
> +ovn-nbctl ls-add ls0
> +# Create gateway router
> +ovn-nbctl lr-add lr0
> +# Add router port to gateway router
> +ovn-nbctl lrp-add lr0 lr0-ls0 f0:00:00:00:00:01 192.168.0.1/24
> +ovn-nbctl lsp-add ls0 ls0-lr0 -- set Logical_Switch_Port ls0-lr0 \
> +type=router options:router-port=lr0-ls0 addresses='"f0:00:00:00:00:01"'
> +
> +# Create a localnet port.
> +ovn-nbctl lsp-add ls0 ln_port
> +ovn-nbctl lsp-set-addresses ln_port unknown
> +ovn-nbctl lsp-set-type ln_port localnet
> +ovn-nbctl --wait=hv lsp-set-options ln_port network_name=physnet1
> +
> +# Prepare packets
> +touch empty_expected
> +echo
> "f00108060001080006040001f001c0a80001c0a80001"
> > arp_expected
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl \
> +-- add-br br-phys \
> +-- add-br br-eth0
> +
> +ovn_attach n1 br-phys 192.168.0.10
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch .
> external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif
> options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap])
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl \
> +-- add-br br-phys \
> +-- add-br br-eth0
> +
> +ovn_attach n1 br-phys 192.168.0.20
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch .
> external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif
> options:tx_pcap=hv2/snoopvif-tx.pcap options:rxq_pcap=hv2/snoopvif-rx.pcap])
> +
> +ovn-sbctl dump-flows > sbflows
> +AT_CAPTURE_FILE([sbflows])
> +
> +# Wait until the patch ports are created in hv1 and hv2 to connect br-int to
> br-eth0
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-vsctl show | \
> +grep "Port patch-br-int-to-ln_port" | wc -l`])
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv2])
> +OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
> +grep "Port patch-br-int-to-ln_port" | wc -l`])
> +
> +# Temporarily remove lr0 chassis
> +AT_CHECK([ovn-nbctl remove logical_router lr0 options chassis])
> +
> +reset_pcap_file() {
> +local hv=$1
> +local iface=$2
> +local pcap_file=$3
> +as $hv
> +ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \
> +options:rxq_pcap=dummy-rx.pcap
> +rm -f ${pcap_file}*.pcap
> +ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \
> +options:rxq_pcap=${pcap_file}-rx.pcap
> +}
> +
> +reset_pcap_file hv1 snoopvif hv1/snoopvif
> +reset_pcap_file hv2 snoopvif hv2/snoopvif
> +
> +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1)
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +OVS_WAIT_UNTIL([
> +ls0_lr0=$(ovn-sbctl --bare --columns chassis list port_binding ls0-lr0)
> +test "$ls0_lr0" = $hv1_uuid
> +])
Since GARPs are periodic, you should give controllers some time (with
a sleep?) to produce the GARPs.
> +
> +OVN_CHECK_PACKETS([hv1/snoopvif-tx.pcap], [arp_expected])
> +OVN_CHECK_PACKETS([hv2/snoopvif-tx.pcap], [empty_expected])
> +
> +# Temporarily remove lr0 chassis
> +AT_CHECK([ovn-nbctl remove logical_router lr0 options chassis])
> +
> +reset_pcap_file hv1 snoopvif
Re: [ovs-dev] [PATCH v2] ovsdb-idl: Support change tracking of write-only columns.
On Tue, Apr 19, 2022 at 12:49 AM Dumitru Ceara wrote:
>
> On 4/16/22 00:41, Han Zhou wrote:
> > On Fri, Apr 15, 2022 at 1:45 PM Dumitru Ceara wrote:
> >>
> >> On 4/15/22 20:25, Han Zhou wrote:
> >>> On Fri, Apr 15, 2022 at 7:22 AM Dumitru Ceara
wrote:
>
> At a first glance, change tracking should never be allowed for
> write-only columns. However, some clients (e.g., ovn-northd) that
are
> mostly exclusive writers of a database, use change tracking to avoid
> duplicating the IDL row records into a local cache when implementing
> incremental processing.
> >>>
> >>> Hi Dumitru,
> >>>
> >>
> >> Hi Han,
> >>
> >>> It is still not clear to me why ovn-northd would need change-tracking
> > for
> >>> the write-only columns. It didn't require change tracking before using
> > the
> >>> incremental processing engine. In theory, to my understanding,
> >>> change-tracking is required only if the column is an input to some of
> > the
> >>> engine nodes, so that it needs to be alerted when there is a change in
> > the
> >>> database. If the column is write-only to ovn-northd, it means it
doesn't
> >>> care about any change of the column from the DB, but blindly writes to
> > the
> >>> column regardless of the current value. I checked the patch [0], which
> >>> mentioned that
> >>>
> >>> Such nodes are primary inputs to the northd incremental processing
> >>> engine and without proper update processing for Southbound tables,
> >>> northd might fail to timely reconcile the contents of the
Southbound
> >>> database.
> >>>
> >>> I am confused that, if the columns were write-only, how come they
become
> >>> inputs to northd and requires reconcile upon them? Does it mean that
> > they
> >>> were misused before I-P, that they were in fact read/write columns to
> >>> ovn-northd?
> >>
> >> I think it's actually a bit more complicated than this.
> >>
> >> Even for pure write-only tables/columns we have a problem due to the
> >> fact that clustered ovsdb offers at-least-once consistency [2].
> >>
> >> We actually hit the case in which a single northd transaction to insert
> >> a SB.Load_Balancer record resulted in two identical rows being added to
> >> the database, both of them pointing to the same logical switch datapath
> >> binding [3]. When that logical switch was deleted northd would fail to
> >> remove the duplicate from the SB causing a referential integrity
> >> violation due to the dangling reference.
> >>
> >> Arguably the schema for load balancers is not ideal because it doesn't
> >> defines an index on "name". Nevertheless, that's not something we can
> >> change easily because it will (quite likely with raft) break existing
> >> deployments. And it applies to other tables in the NB/SB schema.
> >>
> >> The fix was relatively straighforward though [4], and it meant fixing
> >> the way northd reconciles the SB.Load_balancers.
> >>
> >> This is were I-P became an issue. Consider the current main branch
> >> code with commit e4d6d3455baf ("ovn-northd: Enable change tracking for
> >> all SB tables.") reverted. In a sandbox we do:
> >>
> >> $ ovn-nbctl ls-add ls
> >> $ ovn-nbctl lb-add lb1 1.1.1.1:1000 2.2.2.2:2000 -- ls-lb-add ls lb1
> >>
> >> # At this point the SB.LB table looks ok:
> >> $ ovn-sbctl list load_balancer
> >
> >
> >> _uuid : 71d07ce2-d8ad-44f5-8fb6-9e4f5ef7ea92
> >> datapaths : [29d576ef-31f5-485b-acb7-190373ef74c3]
> >> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
> >> name: lb1
> >> options : {hairpin_orig_tuple="true"}
> >> protocol: tcp
> >> vips: {"1.1.1.1:1000"="2.2.2.2:2000"}
> >>
> >> # Simulate the bug in [3]
> >> $ ovn-sbctl create load_balancer name=lb1
> > external_ids:lb_id=c2206d6a-6939-47cc-92de-554d2bc163b0
> >>
> >> # Even with the northd fix [4], due to the fact that the LB table is
> >> # write-only, northd doesn't wake up so we end up with the duplicate
> >> # staying in the SB for an indeterminate time.
> >> $ ovn-sbctl list load_balancer
> >> _uuid : 71d07ce2-d8ad-44f5-8fb6-9e4f5ef7ea92
> >> datapaths : [29d576ef-31f5-485b-acb7-190373ef74c3]
> >> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
> >> name: lb1
> >> options : {hairpin_orig_tuple="true"}
> >> protocol: tcp
> >> vips: {"1.1.1.1:1000"="2.2.2.2:2000"}
> >>
> >> _uuid : 0bff0f83-234a-4631-ab88-2348b9d07d1f
> >> datapaths : []
> >> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
> >> name: lb1
> >> options : {}
> >> protocol: []
> >> vips: {}
> >>
> >> # Only when we generate an input for a "non-write-only" column the
SB.LB
> >> # is reconciled, because the "northd" I-P node runs.
> >> $ ovn-nbctl --wait=sb sync
> >> $ ovn-sbctl list load_balancer
> >> _uuid
[ovs-dev] [PATCH] dpif-netdev/mfex avx512: fix ubsan shift on bitmask
This commit ensures the compiler knows the 1 bit is an unsigned 32-bit
wide 1 bit, keeping undefined sanitizer happy at runtime.
Fixes: 250ceddcc ("dpif-netdev/mfex: Add AVX512 based optimized miniflow
extract")
Signed-off-by: Harry van Haaren
---
lib/dpif-netdev-extract-avx512.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index c1c1fefb6..8cd8b6c6e 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -619,7 +619,7 @@ mfex_avx512_process(struct dp_packet_batch *packets,
};
/* This packet has its miniflow created, add to hitmask. */
-hitmask |= 1 << i;
+hitmask |= 1ULL << i;
}
return hitmask;
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev-avx512: fix ubsan shift error in bitmasks
The code changes here are to handle (1 << i) shifts where 'i' is the
packet index in the batch, and 1 << 31 is an overflow of the signed '1'.
Fixed by adding ULL suffix to the 1 character, ensuring compiler knows
the 1 is unsigned (and 32-bits minimum). Undefined Behaviour sanitizer
is now happy with the shifts at runtime.
Suggested-by: Ilya Maximets
Signed-off-by: Harry van Haaren
---
Thanks Ilya for the detail in the email - reworked as commit message;
https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html
---
lib/dpif-netdev-avx512.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index b7131ba3f..fdefee230 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -183,7 +183,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
* classifed by vector mfex else do a scalar miniflow extract
* for that packet.
*/
-bool mfex_hit = !!(mf_mask & (1 << i));
+bool mfex_hit = !!(mf_mask & (1ULL << i));
/* Check for a partial hardware offload match. */
if (hwol_enabled) {
@@ -204,7 +204,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
pkt_meta[i].bytes = dp_packet_size(packet);
phwol_hits++;
-hwol_emc_smc_hitmask |= (1 << i);
+hwol_emc_smc_hitmask |= (1ULL << i);
continue;
}
}
@@ -227,7 +227,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
if (f) {
rules[i] = >cr;
emc_hits++;
-hwol_emc_smc_hitmask |= (1 << i);
+hwol_emc_smc_hitmask |= (1ULL << i);
continue;
}
}
@@ -237,7 +237,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
if (f) {
rules[i] = >cr;
smc_hits++;
-smc_hitmask |= (1 << i);
+smc_hitmask |= (1ULL << i);
continue;
}
}
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2] northd: Avoid looking up port peers when not needed.
There's no need to always lookup a port's peer in ovn_port_get_peer().
Just use what was already stored in op->peer instead.
Also, factor out addition of router ports to a logical switch's
ovn_datapath and use x2nrealloc().
Signed-off-by: Dumitru Ceara
---
v2: Changed patch according to Mark's review:
- modify ovn_port_get_peer() to check if op->peer is already set.
- rephrased the commit log accordingly.
---
northd/northd.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index bcd36bbaa..530bbc61b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -601,6 +601,7 @@ struct ovn_datapath {
/* Logical switch data. */
struct ovn_port **router_ports;
size_t n_router_ports;
+size_t n_allocated_router_ports;
struct hmap port_tnlids;
uint32_t port_key_hint;
@@ -1081,6 +1082,17 @@ ovn_datapath_from_sbrec(const struct hmap *datapaths,
return NULL;
}
+static void
+ovn_datapath_add_router_port(struct ovn_datapath *od, struct ovn_port *op)
+{
+if (od->n_router_ports == od->n_allocated_router_ports) {
+od->router_ports = x2nrealloc(od->router_ports,
+ >n_allocated_router_ports,
+ sizeof *od->router_ports);
+}
+od->router_ports[od->n_router_ports++] = op;
+}
+
static bool
lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
{
@@ -1815,6 +1827,10 @@ ovn_port_get_peer(const struct hmap *ports, struct
ovn_port *op)
return NULL;
}
+if (op->peer) {
+return op->peer;
+}
+
const char *peer_name = smap_get(>nbsp->options, "router-port");
if (!peer_name) {
return NULL;
@@ -2657,12 +2673,9 @@ join_logical_ports(struct northd_input *input_data,
continue;
}
+ovn_datapath_add_router_port(op->od, op);
peer->peer = op;
op->peer = peer;
-op->od->router_ports = xrealloc(
-op->od->router_ports,
-sizeof *op->od->router_ports * (op->od->n_router_ports + 1));
-op->od->router_ports[op->od->n_router_ports++] = op;
/* Fill op->lsp_addrs for op->nbsp->addresses[] with
* contents "router", which was skipped in the loop above. */
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4 00/15] Support multiple requested-chassis
On Tue, Mar 29, 2022 at 8:47 PM Ihar Hrachyshka wrote: > > This version of the series switched to supporting multiple chassis set in > requested-chassis option. This allows for more than two chassis > specified for the same port. > > Also, this series includes a patch that allows to disable tunneling > enforcement for ports with multiple chassis set in requested-chassis. > (This is useful when tunneling network is MTU constrained.) > > v0: initial draft (single patch) > v1: split into pieces > v1: renamed options: migration-destination -> > requested-additional-chassis, > migration-unblocked -> > additional-chassis-activated > v1: introduced options:activation-strategy=rarp to allow for other > strategies / having default no-op strategy > v1: implement in-memory port-activated tracking to avoid races > v1: numerous code cleanup / bug fixes > v1: special handling for localnet attached switches > v2: added ddlog implementation > v2: re-inject RARP packet after vswitch updates flows > v3: re-sent as a single series > v4: redesign to reuse requested-chassis option > v4: support >2 chassis per port > v4: allow to disable tunneling enforcement when n_chassis >= 2 > > Ihar Hrachyshka (15): > Introduce chassis_is_vtep > northd: introduce separate function to look up chassis > northd: separate code for nb->sb port binding chassis update > Pass chassis and encap into get_port_binding_tun > Introduce match_outport_dp_and_port_keys in physical.c > Split code to set zone info into put_zones_ofpacts > Use get_port_binding_tun instead of chassis_tunnel_find > Tag all packets that arrived from a tunnel as LOCAL_ONLY > Update port-up on main chassis only > Support LSP:options:requested-chassis as a list > Clone packets to both port chassis > Implement RARP activation strategy for ports > Reinject RARP packet when activation-strategy=rarp > Allow to disable tunneling enforcement for multi-chassis port > Update NEWS file about new live migration options Hi Ihar, Thanks for the patch series. I have a few comments. 1) Do you think we really need to support port_binding multiple encaps for this use case ? If you see this commit - https://github.com/ovn-org/ovn/commit/dd527a283cd8b67ca481374302a630f5635de6ac encaps for port binding was added for a specific use case. If CMS desires to configure multiple encaps for a logical port, then I think its Ok to not support multiple requested-chassis for such ports. What do you think ? Do you see any use case for this in openstack ? IMO supporting multiple encaps is complicating the code in binding.c and I'd prefer if we can avoid it. 2) In patch 12 (RARP activation strategy), ovn-controller is programming OF flows in logical flow space (i.e table 8). I think it's better to avoid it. Instead I'd suggest adding these flows in the new physical table - table 73 (see lflow.h). You can modify the flow in table 0 to resubmit to table 73 if the ovs interface port is created on additional chassis. And table 73 can have a controller action for rarp packet and a generic drop flow. 2.a ) In the proposed patch series, pinctrl thread when it receives a packet due to rarp, it deletes the flows. Instead I'd suggest doing the below. In table 73, add a flow with the match = rarp and action = controller(..), learn(...) The first action will send the packet to the controller. When the pinctrl thread receives it, it can tell the main thread to set the option additional-chassis-activated for the port binding. The second OF action "learn" can add a new higher priority flow to resubmit the packets to table 8. This way, pinctrl thread doesn't need to modify or delete any openflows when it receives a rarp activation packet. What do you think ? One use case of learn which ovn-controller uses - https://github.com/ovn-org/ovn/blob/main/controller/lflow.c#L1752 Also I don't think there is a need to set the "pause" flag for the OVN action - activation_strategy_rarp Do you see any problem with this ? 3) Patch 11 (Clone packets to both port chassis) is adding flows in table 38 to tunnel the packet to other requested chassis. Table 38 is meant for local output and I think it should not do this. Table 37 is meant for tunnelling. If suppose a logical port has 2 requested chassis - ch1 and ch2, and if there is ovs port for this logical port on both ch1 and ch2, then I'd suggest adding/modifying the flow in table 37 to tunnel to the other requested chassis and then do a resubmit to 38. These are the comments I had at this point. Let me know what you think. Thanks Numan > > NEWS|3 + > controller/binding.c| 284 -- > controller/binding.h|5 + > controller/if-status.c | 15 +- > controller/if-status.h |1 + > controller/lport.c | 42 +- >
[ovs-dev] [PATCH ovn] northd: Add new NB_Global.options:default_acl_drop option.
This option changes how logical switch ACL related flows are generated
such that the following behavior is ensured:
a. If a logical switch has no ACL applied to it (either directly or
indirectly via a port group) then traffic is always allowed in the
ls_in_acl, ls_in_acl_after_lb, ls_out_acl stages.
b. If a logical switch has ACLs applied (directly or indirectly) and
NB_Global.options:default_acl_drop is set to 'false', then traffic that
doesn't match any ACL in the ls_in_acl, ls_in_acl_after_lb, ls_out_acl
stages is allowed to advance to the next step in the processing
pipeline.
c. If a logical switch has *any* ACL applied (directly or indirectly)
and NB_Global.options:default_acl_drop is set to 'true', then a default
lowest-priority rule is added to the ls_in_acl, ls_in_acl_after_lb,
ls_out_acl stages to drop traffic that is not matched by any ACLs.
The goal of the feature is to simplify the configuration of the ACLs and
port groups for CMSs that require a default-deny firewall
implementation. One such example is with OpenStack security groups
which, when enabled, implicitly drop all not explicitly allowed traffic.
Until now the CMS had to add all logical ports corresponding to VMs in a
network to a single, huge, default-drop-port-group and apply a single
drop ACL to the port group.
With this new feature, the CMS can enable 'default_acl_drop', and punch
holes for traffic that needs to be allowed. The resulting NB and SB
configuration is also reduced in size.
Reported-by: Daniel Alvarez Sanchez
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1947807
Signed-off-by: Dumitru Ceara
---
v1:
- Removed RFC tag.
- Added flows for the stateful-ACL case as pointed by Han.
---
NEWS| 2 +
northd/northd.c | 50 -
northd/ovn-northd.8.xml | 28 ++-
ovn-nb.xml | 8 +
tests/ovn-northd.at | 483 +++-
5 files changed, 551 insertions(+), 20 deletions(-)
diff --git a/NEWS b/NEWS
index c881764a6..dbe89e9cf 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,8 @@ Post v22.03.0
- Replaced the usage of masked ct_label by ct_mark in most cases to work
better with hardware-offloading.
- Support NAT for logical routers with multiple distributed gateway ports.
+ - Add global option (NB_Global.options:default_acl_drop) to enable
+implicit drop behavior on logical switches with ACLs applied.
OVN v22.03.0 - 11 Mar 2022
--
diff --git a/northd/northd.c b/northd/northd.c
index bcd36bbaa..43b028c2c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -74,6 +74,12 @@ static struct eth_addr svc_monitor_mac_ea;
* Otherwise, it will avoid using it. The default is true. */
static bool use_ct_inv_match = true;
+/* If this option is 'true' northd will implicitly add a lowest-priority
+ * drop rule in the ACL stage of logical switches that have at least one
+ * ACL.
+ */
+static bool default_acl_drop;
+
#define MAX_OVN_TAGS 4096
/* Pipeline stages. */
@@ -6617,6 +6623,7 @@ static void
build_acls(struct ovn_datapath *od, struct hmap *lflows,
const struct hmap *port_groups, const struct shash *meter_groups)
{
+const char *default_acl_action = default_acl_drop ? "drop;" : "next;";
bool has_stateful = od->has_stateful_acl || od->has_lb_vip;
struct ds match = DS_EMPTY_INITIALIZER;
struct ds actions = DS_EMPTY_INITIALIZER;
@@ -6628,22 +6635,34 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
*
* A related rule at priority 1 is added below if there
* are any stateful ACLs in this datapath. */
-if (!od->has_acls && !od->has_lb_vip) {
-ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "1", "next;");
-ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "1", "next;");
+if (!od->has_acls) {
+if (!od->has_lb_vip) {
+ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "1",
+ "next;");
+ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "1",
+ "next;");
+} else {
+ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "1", "next;");
+ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "1", "next;");
+}
+ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB, 0, "1", "next;");
} else {
-ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1", "next;");
-ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1", "next;");
+ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 0, "1",
+ default_acl_action);
+ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 0, "1",
+ default_acl_action);
+ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB, 0, "1",
+ default_acl_action);
}
-ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB, 0, "1", "next;");
if (has_stateful) {
/* Ingress and Egress ACL Table
Re: [ovs-dev] [PATCH ovn 0/3] Use newest OVS version to fix Undefined Behavior
On 4/19/22 05:06, Adrian Moreno wrote: On 4/8/22 21:03, Mark Michelson wrote: Hi Adrian, The patches look correct to me, however when I tried to apply them locally, I hit a failure: Applying: treewide: remove next variable in _SAFE loops error: sha1 information is lacking or useless (controller/ofctrl.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0003 treewide: remove next variable in _SAFE loops When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort" I'm pretty sure this just means it needs a rebase. If you can rebase and re-upload, that would be great. Sure, I'll rebase and post v2. One question: Patches 1 and 2 are the minimum required to fix the undefined behavior and update to the newer OVS source. Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. Should I send another patch updating LTS branches with only patches 1 and 2 and the correspondent bump in ovs version? The only reason to do that is if the patches you provide for the main branch do not apply cleanly when applied to branch-22.03. Currently branch-22.03 is the only LTS OVN branch, and it is the most recent version of OVN, so hopefully the patches you create for main will apply cleanly. Thanks, Adrián Thanks, Mark On 4/6/22 10:10, Adrian Moreno wrote: A series has been recently introduced in OVS that has two main benefits: - Undefined Behavior in iterator loops is removed. This enforces some restrictions on how this macros can be used, namely: * User-provided iterator variable is set to NULL after the loop ends normally * In _SAFE version of the loop, it's not always safe to use the 'next' variable. When it would point to the list's head, it is instead set to NULL by the iteration macros. - _SAFE version of the iterator macros now do not require the user to provide a 'next' variable leading to cleaner and less error prone code. This series bumps ovs to the latest HEAD in master branch and adapts the code accordingly. Adrian Moreno (3): treewide: bump ovs and fix problematic loops parallel-hmap: rewrite iterator using multivar helpers treewide: remove next variable in _SAFE loops controller-vtep/binding.c | 4 +- controller-vtep/gateway.c | 4 +- controller-vtep/vtep.c | 4 +- controller/binding.c | 21 +++--- controller/encaps.c | 4 +- controller/if-status.c | 17 ++--- controller/lflow-cache.c | 3 +- controller/lflow-conj-ids.c | 18 +++-- controller/lflow.c | 36 +- controller/ofctrl-seqno.c | 4 +- controller/ofctrl.c | 84 +++--- controller/ovn-controller.c | 20 +++--- controller/patch.c | 4 +- controller/physical.c | 4 +- controller/pinctrl.c | 69 +- controller/vif-plug.c | 8 +-- ic/ovn-ic.c | 12 ++-- lib/actions.c | 2 +- lib/expr.c | 51 +++-- lib/extend-table.c | 20 +++--- lib/extend-table.h | 4 +- lib/ovn-parallel-hmap.h | 10 +-- lib/ovn-util.c | 2 +- lib/vif-plug-provider.c | 8 +-- northd/northd.c | 139 +--- northd/ovn-northd-ddlog.c | 4 +- northd/ovn-northd.c | 16 ++--- ovs | 2 +- utilities/ovn-ic-nbctl.c | 4 +- utilities/ovn-ic-sbctl.c | 4 +- utilities/ovn-nbctl.c | 14 ++-- utilities/ovn-sbctl.c | 4 +- utilities/ovn-trace.c | 12 ++-- 33 files changed, 299 insertions(+), 313 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev: ct maxconns config persistence
Max allowed conntrack entries is configurable with
'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
this configuration is expected to survive from host reboot.
Signed-off-by: lic121
---
lib/dpctl.man | 3 ++-
lib/dpif-netdev.c | 10 ++
tests/system-traffic.at | 10 ++
vswitchd/vswitch.xml| 7 +++
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/lib/dpctl.man b/lib/dpctl.man
index c100d0a..2cfc4f2 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -343,7 +343,8 @@ system due to connection tracking or simply limiting
connection
tracking. If the number of connections is already over the new maximum
limit request then the new maximum limit will be enforced when the
number of connections decreases to that limit, which normally happens
-due to connection expiry. Only supported for userspace datapath.
+due to connection expiry. Only supported for userspace datapath. This
+command is deprecated by ovsdb cfg other_config:ct-maxconns.
.
.TP
\*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6764343..c518d30 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4827,6 +4827,16 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
}
}
+uint32_t ct_maxconns, cur_maxconns;
+ct_maxconns = smap_get_int(other_config, "ct-maxconns", UINT32_MAX);
+/* Leave runtime value as it is when cfg is removed. */
+if (ct_maxconns < UINT32_MAX) {
+conntrack_get_maxconns(dp->conntrack, _maxconns);
+if (ct_maxconns != cur_maxconns) {
+conntrack_set_maxconns(dp->conntrack, ct_maxconns);
+}
+}
+
bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
bool cur_smc;
atomic_read_relaxed(>smc_enable_db, _smc);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4a7fa49..00fefc5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
10
])
+AT_CHECK([ovs-vsctl set Open_vswitch . other_config:ct-maxconns=20], [0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
+AT_CHECK([ovs-vsctl remove Open_vswitch . other_config ct-maxconns], [0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 0c66326..ec634d5 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -183,6 +183,13 @@
+
+The maximum number of connection tracker entries allowed in the
+datapath. Only supported for userspace datapath. This deprecates
+"ovs-appctl dpctl/ct-set-maxconns" command.
+
+
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 0/3] Use newest OVS version to fix Undefined Behavior
On 4/8/22 21:03, Mark Michelson wrote: Hi Adrian, The patches look correct to me, however when I tried to apply them locally, I hit a failure: Applying: treewide: remove next variable in _SAFE loops error: sha1 information is lacking or useless (controller/ofctrl.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0003 treewide: remove next variable in _SAFE loops When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort" I'm pretty sure this just means it needs a rebase. If you can rebase and re-upload, that would be great. Sure, I'll rebase and post v2. One question: Patches 1 and 2 are the minimum required to fix the undefined behavior and update to the newer OVS source. Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. Should I send another patch updating LTS branches with only patches 1 and 2 and the correspondent bump in ovs version? Thanks, Adrián Thanks, Mark On 4/6/22 10:10, Adrian Moreno wrote: A series has been recently introduced in OVS that has two main benefits: - Undefined Behavior in iterator loops is removed. This enforces some restrictions on how this macros can be used, namely: * User-provided iterator variable is set to NULL after the loop ends normally * In _SAFE version of the loop, it's not always safe to use the 'next' variable. When it would point to the list's head, it is instead set to NULL by the iteration macros. - _SAFE version of the iterator macros now do not require the user to provide a 'next' variable leading to cleaner and less error prone code. This series bumps ovs to the latest HEAD in master branch and adapts the code accordingly. Adrian Moreno (3): treewide: bump ovs and fix problematic loops parallel-hmap: rewrite iterator using multivar helpers treewide: remove next variable in _SAFE loops controller-vtep/binding.c | 4 +- controller-vtep/gateway.c | 4 +- controller-vtep/vtep.c | 4 +- controller/binding.c | 21 +++--- controller/encaps.c | 4 +- controller/if-status.c | 17 ++--- controller/lflow-cache.c | 3 +- controller/lflow-conj-ids.c | 18 +++-- controller/lflow.c | 36 +- controller/ofctrl-seqno.c | 4 +- controller/ofctrl.c | 84 +++--- controller/ovn-controller.c | 20 +++--- controller/patch.c | 4 +- controller/physical.c | 4 +- controller/pinctrl.c | 69 +- controller/vif-plug.c | 8 +-- ic/ovn-ic.c | 12 ++-- lib/actions.c | 2 +- lib/expr.c | 51 +++-- lib/extend-table.c | 20 +++--- lib/extend-table.h | 4 +- lib/ovn-parallel-hmap.h | 10 +-- lib/ovn-util.c | 2 +- lib/vif-plug-provider.c | 8 +-- northd/northd.c | 139 +--- northd/ovn-northd-ddlog.c | 4 +- northd/ovn-northd.c | 16 ++--- ovs | 2 +- utilities/ovn-ic-nbctl.c | 4 +- utilities/ovn-ic-sbctl.c | 4 +- utilities/ovn-nbctl.c | 14 ++-- utilities/ovn-sbctl.c | 4 +- utilities/ovn-trace.c | 12 ++-- 33 files changed, 299 insertions(+), 313 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: Avoid looking up port peers when not needed.
On 4/18/22 19:32, Mark Michelson wrote:
> Hi Dumitru,
>
Hi Mark,
Thanks for reviewing this patch!
> This change makes it so that ovn_port_get_peer() is only relevant before
> op->peer is set; otherwise, you just use op->peer directly. This means
> that there is only one place in the code now that calls
> ovn_port_get_peer(). I have a few ideas here. In order of preference:
>
> 1) Instead of removing all the calls to ovn_port_get_peer(), modify
> ovn_port_get_peer() to attempt to return op->peer if it is non-NULL.
> Otherwise, it can fall back to looking up by router-port. This way
> developers always call ovn_port_get_peer() no matter the circumstances,
> lessening the cognitive load.
I will do this.
>
> 2) Get rid of ovn_port_get_peer() and put its logic in-line in
> join_logical_ports(). This gets rid of a single-use function and keeps
> other developers from calling it unnecessarily.
>
I don't really like this option because join_logical_ports() is really
long and complex already.
> 3) Leave your code as-is but add a comment to ovn_port_get_peer() to
> explain that it is unnecessary the vast majority of the time.
Normally, I'd prefer this solution, but that's because the code
populating ovn_port and ovn_datapath structures should be, in my
opinion, separated modules, distinct from the code that uses ovn_port
and ovn_datapath. That's not currently the case though.
>
> What do you think?
>
I'll post v2, implementing option "1)".
> Mark
>
Thanks,
Dumitru
> On 4/12/22 11:26, Dumitru Ceara wrote:
>> There's no need to call ovn_port_get_peer() to find the peer port of a
>> logical switch port that's connected to a logical router. We already
>> store those in op->peer.
>>
>> Also, factor out addition of router ports to a logical switch's
>> ovn_datapath and use x2nrealloc().
>>
>> Signed-off-by: Dumitru Ceara
>> ---
>> Spotted during code inspection. I don't really think it will improve
>> performance but it can't hurt and it should make the code a bit more
>> clear.
>> ---
>> northd/northd.c | 42 +++---
>> 1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 01ae7d7fd7..6308259159 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -601,6 +601,7 @@ struct ovn_datapath {
>> /* Logical switch data. */
>> struct ovn_port **router_ports;
>> size_t n_router_ports;
>> + size_t n_allocated_router_ports;
>> struct hmap port_tnlids;
>> uint32_t port_key_hint;
>> @@ -1081,6 +1082,17 @@ ovn_datapath_from_sbrec(const struct hmap
>> *datapaths,
>> return NULL;
>> }
>> +static void
>> +ovn_datapath_add_router_port(struct ovn_datapath *od, struct ovn_port
>> *op)
>> +{
>> + if (od->n_router_ports == od->n_allocated_router_ports) {
>> + od->router_ports = x2nrealloc(od->router_ports,
>> + >n_allocated_router_ports,
>> + sizeof *od->router_ports);
>> + }
>> + od->router_ports[od->n_router_ports++] = op;
>> +}
>> +
>> static bool
>> lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
>> {
>> @@ -2657,12 +2669,9 @@ join_logical_ports(struct northd_input
>> *input_data,
>> continue;
>> }
>> + ovn_datapath_add_router_port(op->od, op);
>> peer->peer = op;
>> op->peer = peer;
>> - op->od->router_ports = xrealloc(
>> - op->od->router_ports,
>> - sizeof *op->od->router_ports *
>> (op->od->n_router_ports + 1));
>> - op->od->router_ports[op->od->n_router_ports++] = op;
>> /* Fill op->lsp_addrs for op->nbsp->addresses[] with
>> * contents "router", which was skipped in the loop
>> above. */
>> @@ -11142,8 +11151,8 @@ build_ip_routing_pre_flows_for_lrouter(struct
>> ovn_datapath *od,
>> * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member.
>> */
>> static void
>> -build_ip_routing_flows_for_lrouter_port(
>> - struct ovn_port *op, const struct hmap *ports, struct hmap
>> *lflows)
>> +build_ip_routing_flows_for_lrouter_port(struct ovn_port *op,
>> + struct hmap *lflows)
>> {
>> if (op->nbrp) {
>> @@ -11161,14 +11170,13 @@ build_ip_routing_flows_for_lrouter_port(
>> >nbrp->header_, false,
>> ROUTE_PRIO_OFFSET_CONNECTED);
>> }
>> } else if (lsp_is_router(op->nbsp)) {
>> - struct ovn_port *peer = ovn_port_get_peer(ports, op);
>> + struct ovn_port *peer = op->peer;
>> if (!peer || !peer->nbrp || !peer->lrp_networks.n_ipv4_addrs) {
>> return;
>> }
>> for (int i = 0; i < op->od->n_router_ports; i++) {
>> - struct ovn_port *router_port = ovn_port_get_peer(
>> - ports, op->od->router_ports[i]);
>> +
Re: [ovs-dev] [PATCH v2] ovsdb-idl: Support change tracking of write-only columns.
On 4/16/22 00:41, Han Zhou wrote:
> On Fri, Apr 15, 2022 at 1:45 PM Dumitru Ceara wrote:
>>
>> On 4/15/22 20:25, Han Zhou wrote:
>>> On Fri, Apr 15, 2022 at 7:22 AM Dumitru Ceara wrote:
At a first glance, change tracking should never be allowed for
write-only columns. However, some clients (e.g., ovn-northd) that are
mostly exclusive writers of a database, use change tracking to avoid
duplicating the IDL row records into a local cache when implementing
incremental processing.
>>>
>>> Hi Dumitru,
>>>
>>
>> Hi Han,
>>
>>> It is still not clear to me why ovn-northd would need change-tracking
> for
>>> the write-only columns. It didn't require change tracking before using
> the
>>> incremental processing engine. In theory, to my understanding,
>>> change-tracking is required only if the column is an input to some of
> the
>>> engine nodes, so that it needs to be alerted when there is a change in
> the
>>> database. If the column is write-only to ovn-northd, it means it doesn't
>>> care about any change of the column from the DB, but blindly writes to
> the
>>> column regardless of the current value. I checked the patch [0], which
>>> mentioned that
>>>
>>> Such nodes are primary inputs to the northd incremental processing
>>> engine and without proper update processing for Southbound tables,
>>> northd might fail to timely reconcile the contents of the Southbound
>>> database.
>>>
>>> I am confused that, if the columns were write-only, how come they become
>>> inputs to northd and requires reconcile upon them? Does it mean that
> they
>>> were misused before I-P, that they were in fact read/write columns to
>>> ovn-northd?
>>
>> I think it's actually a bit more complicated than this.
>>
>> Even for pure write-only tables/columns we have a problem due to the
>> fact that clustered ovsdb offers at-least-once consistency [2].
>>
>> We actually hit the case in which a single northd transaction to insert
>> a SB.Load_Balancer record resulted in two identical rows being added to
>> the database, both of them pointing to the same logical switch datapath
>> binding [3]. When that logical switch was deleted northd would fail to
>> remove the duplicate from the SB causing a referential integrity
>> violation due to the dangling reference.
>>
>> Arguably the schema for load balancers is not ideal because it doesn't
>> defines an index on "name". Nevertheless, that's not something we can
>> change easily because it will (quite likely with raft) break existing
>> deployments. And it applies to other tables in the NB/SB schema.
>>
>> The fix was relatively straighforward though [4], and it meant fixing
>> the way northd reconciles the SB.Load_balancers.
>>
>> This is were I-P became an issue. Consider the current main branch
>> code with commit e4d6d3455baf ("ovn-northd: Enable change tracking for
>> all SB tables.") reverted. In a sandbox we do:
>>
>> $ ovn-nbctl ls-add ls
>> $ ovn-nbctl lb-add lb1 1.1.1.1:1000 2.2.2.2:2000 -- ls-lb-add ls lb1
>>
>> # At this point the SB.LB table looks ok:
>> $ ovn-sbctl list load_balancer
>
>
>> _uuid : 71d07ce2-d8ad-44f5-8fb6-9e4f5ef7ea92
>> datapaths : [29d576ef-31f5-485b-acb7-190373ef74c3]
>> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
>> name: lb1
>> options : {hairpin_orig_tuple="true"}
>> protocol: tcp
>> vips: {"1.1.1.1:1000"="2.2.2.2:2000"}
>>
>> # Simulate the bug in [3]
>> $ ovn-sbctl create load_balancer name=lb1
> external_ids:lb_id=c2206d6a-6939-47cc-92de-554d2bc163b0
>>
>> # Even with the northd fix [4], due to the fact that the LB table is
>> # write-only, northd doesn't wake up so we end up with the duplicate
>> # staying in the SB for an indeterminate time.
>> $ ovn-sbctl list load_balancer
>> _uuid : 71d07ce2-d8ad-44f5-8fb6-9e4f5ef7ea92
>> datapaths : [29d576ef-31f5-485b-acb7-190373ef74c3]
>> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
>> name: lb1
>> options : {hairpin_orig_tuple="true"}
>> protocol: tcp
>> vips: {"1.1.1.1:1000"="2.2.2.2:2000"}
>>
>> _uuid : 0bff0f83-234a-4631-ab88-2348b9d07d1f
>> datapaths : []
>> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
>> name: lb1
>> options : {}
>> protocol: []
>> vips: {}
>>
>> # Only when we generate an input for a "non-write-only" column the SB.LB
>> # is reconciled, because the "northd" I-P node runs.
>> $ ovn-nbctl --wait=sb sync
>> $ ovn-sbctl list load_balancer
>> _uuid : 71d07ce2-d8ad-44f5-8fb6-9e4f5ef7ea92
>> datapaths : [29d576ef-31f5-485b-acb7-190373ef74c3]
>> external_ids: {lb_id="c2206d6a-6939-47cc-92de-554d2bc163b0"}
>> name: lb1
>> options : {hairpin_orig_tuple="true"}
>> protocol
