On 27/04/2021 18:29, Han Zhou wrote:
> On Tue, Apr 27, 2021 at 8:43 AM Mark Gray <[email protected]> wrote:
>>
>> On 22/04/2021 21:14, Han Zhou wrote:
>>> For an ACL with match: outport == @PG && ip4.src == $PG_AS, given below
>>> scale:
>>>
>>> P: PG size
>>> LP: number of local lports
>>> D: number of all datapaths (logical switches)
>>> LD: number of datapaths that contain local lports
>>>
>>> With current OVN implementation, the total number of OF flows is:
>>> LP + (P * D) + D
>>>
>>> The reason is, firstly, datapath is not part of the conjunction, so for
>>> each datapath the lflow is reparsed.
>>>
>>> Secondly, although ovn-controller tries to filter out the flows that are
>>> for non-local lports, with the conjunction match, the logic that filters
>>> out non-local flows doesn't work for the conjunction part that doesn't
>>> have the lport in the match (the P * D part). When there is only one
>>> port on each LS it is fine, because no conjunction will be used because
>>> SB port groups are splited per datapath, so each port group would have
>> suggest "split per datapath"
>
> Ack
>
>>> only one port. However, when more than one ports are on each LS the flow
>>> explosion happens.
>>>
>>> This patch deal with the second reason above, by refining the SB port
>>> groups to store only locally bound lports: empty const sets will not
>>> generate any flows. This reduces the related flow number from
>>> LP + (P * D) + D to LP + (P * LD) + LD.
>>>
>>> Since LD is expected to be small, so even if it is a multiplier, the
>>> total number is larged reduced. In particular, in ovn-k8s use cases the
>> suggest "reduced significantly"
>
> Ack
>
>>> LD is always 1, so the formular above becomes LP + P + LD.
>>>
>> s/formular/formula
>
> Ack
>
>>> With a scale of 1k k8s nodes, each has 4 ports for the same PG: P = 4k,
>>> LP = 4, D = 1k, LD = 1. The current implementation generates ~4m flows.
>>> With this patch it becomes only ~4k.
>> Cool!
>>>
>>> Reported-by: Girish Moodalbail <[email protected]>
>>> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-March/381082.html
>>> Reported-by: Dumitru Ceara <[email protected]>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1944098
>>> Signed-off-by: Han Zhou <[email protected]>
>>
>> I tested this as well and it seemed to work as expected.
>
> Thanks for the test!
>
>>> ---
>>> v1->v2: fix memory leaks found by address sanitizer
>>>
>>> controller/binding.c | 20 ++++
>>> controller/binding.h | 9 ++
>>> controller/ovn-controller.c | 217 ++++++++++++++++++++++++++++++------
>>> include/ovn/expr.h | 2 +-
>>> lib/expr.c | 8 +-
>>> tests/ovn.at | 53 +++++++++
>>> tests/test-ovn.c | 12 +-
>>> utilities/ovn-trace.c | 4 +-
>>> 8 files changed, 283 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/controller/binding.c b/controller/binding.c
>>> index 514f5f33f..5aca964cc 100644
>>> --- a/controller/binding.c
>>> +++ b/controller/binding.c
>>> @@ -2987,3 +2987,23 @@ cleanup:
>>>
>>> return b_lport;
>>> }
>>> +
>>> +struct sset *
>>> +binding_collect_local_binding_lports(struct local_binding_data
> *lbinding_data)
>>> +{
>>> + struct sset *lports = xzalloc(sizeof *lports);
>>> + sset_init(lports);
>>> + struct shash_node *shash_node;
>>> + SHASH_FOR_EACH (shash_node, &lbinding_data->lports) {
>>> + struct binding_lport *b_lport = shash_node->data;
>>> + sset_add(lports, b_lport->name);
>>> + }
>>> + return lports;
>>> +}
>>> +
>>> +void
>>> +binding_destroy_local_binding_lports(struct sset *lports)
>>> +{
>>> + sset_destroy(lports);
>>> + free(lports);
>>> +}
>>> diff --git a/controller/binding.h b/controller/binding.h
>>> index 4fc9ef207..31f0352a0 100644
>>> --- a/controller/binding.h
>>> +++ b/controller/binding.h
>>> @@ -128,4 +128,13 @@ void binding_seqno_run(struct local_binding_data
> *lbinding_data);
>>> void binding_seqno_install(struct local_binding_data *lbinding_data);
>>> void binding_seqno_flush(void);
>>> void binding_dump_local_bindings(struct local_binding_data *, struct
> ds *);
>>> +
>>> +/* Generates a sset of lport names from local_binding_data.
>>> + * Note: the caller is responsible for destroying and freeing the
> returned
>>> + * sset, by calling binding_collect_local_binding_lports(). */
>> I think this^ should say binding_destroy_local_binding_lports()?
>
> Oops. My bad.
>
>>> +struct sset *binding_collect_local_binding_lports(struct
> local_binding_data *);
>>> +
>>> +/* Destroy and free the lports sset returned by
>>> + * binding_collect_local_binding_lports(). */
>>> +void binding_destroy_local_binding_lports(struct sset *lports);
>>> #endif /* controller/binding.h */
>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>> index 7320bd56c..c6ba9ff88 100644
>>> --- a/controller/ovn-controller.c
>>> +++ b/controller/ovn-controller.c
>>> @@ -1341,7 +1341,7 @@ addr_sets_init(const struct
> sbrec_address_set_table *address_set_table,
>>> SBREC_ADDRESS_SET_TABLE_FOR_EACH (as, address_set_table) {
>>> expr_const_sets_add(addr_sets, as->name,
>>> (const char *const *) as->addresses,
>>> - as->n_addresses, true);
>>> + as->n_addresses, true, NULL);
>>> }
>>> }
>>>
>>> @@ -1358,7 +1358,7 @@ addr_sets_update(const struct
> sbrec_address_set_table *address_set_table,
>>> } else {
>>> expr_const_sets_add(addr_sets, as->name,
>>> (const char *const *) as->addresses,
>>> - as->n_addresses, true);
>>> + as->n_addresses, true, NULL);
>>> if (sbrec_address_set_is_new(as)) {
>>> sset_add(new, as->name);
>>> } else {
>>> @@ -1367,6 +1367,7 @@ addr_sets_update(const struct
> sbrec_address_set_table *address_set_table,
>>> }
>>> }
>>> }
>>> +
>>> static void
>>> en_addr_sets_run(struct engine_node *node, void *data)
>>> {
>>> @@ -1415,20 +1416,72 @@ addr_sets_sb_address_set_handler(struct
> engine_node *node, void *data)
>>> }
>>>
>>> struct ed_type_port_groups{
>>> - struct shash port_groups;
>>> + /* A copy of SB port_groups, each converted as a sset for
> efficient lport
>>> + * lookup. */
>>> + struct shash port_group_ssets;
>>> +
>>> + /* Const sets containing local lports, used for expr parsing. */
>>> + struct shash port_groups_cs_local;
>>> +
>>> bool change_tracked;
>>> struct sset new;
>>> struct sset deleted;
>>> struct sset updated;
>>> };
>>>
>>> +static void
>>> +port_group_ssets_add_or_update(struct shash *port_group_ssets,
>>> + const struct sbrec_port_group *pg)
>>> +{
>>> + struct sset *lports = shash_find_data(port_group_ssets, pg->name);
>>> + if (lports) {
>>> + sset_clear(lports);
>>> + } else {
>>> + lports = xzalloc(sizeof *lports);
>>> + sset_init(lports);
>>> + shash_add(port_group_ssets, pg->name, lports);
>>> + }
>>> +
>>> + for (size_t i = 0; i < pg->n_ports; i++) {
>>> + sset_add(lports, pg->ports[i]);
>>> + }
>>> +}
>>> +
>>> +static void
>>> +port_group_ssets_delete(struct shash *port_group_ssets,
>>> + const char *pg_name)
>>> +{
>>> + struct shash_node *node = shash_find(port_group_ssets, pg_name);
>>> + if (node) {
>>> + struct sset *lports = node->data;
>>> + shash_delete(port_group_ssets, node);
>>> + sset_destroy(lports);
>>> + free(lports);
>>> + }
>>> +}
>>> +
>>> +/* Delete and free all ssets in port_group_ssets, but not
>>> + * destroying the shash itself. */
>>> +static void
>>> +port_group_ssets_clear(struct shash *port_group_ssets)
>>> +{
>>> + struct shash_node *node, *next;
>>> + SHASH_FOR_EACH_SAFE (node, next, port_group_ssets) {
>>> + struct sset *lports = node->data;
>>> + shash_delete(port_group_ssets, node);
>>> + sset_destroy(lports);
>>> + free(lports);
>>> + }
>>> +}
>>> +
>>> static void *
>>> en_port_groups_init(struct engine_node *node OVS_UNUSED,
>>> struct engine_arg *arg OVS_UNUSED)
>>> {
>>> struct ed_type_port_groups *pg = xzalloc(sizeof *pg);
>>>
>>> - shash_init(&pg->port_groups);
>>> + shash_init(&pg->port_group_ssets);
>>> + shash_init(&pg->port_groups_cs_local);
>>> pg->change_tracked = false;
>>> sset_init(&pg->new);
>>> sset_init(&pg->deleted);
>>> @@ -1440,41 +1493,52 @@ static void
>>> en_port_groups_cleanup(void *data)
>>> {
>>> struct ed_type_port_groups *pg = data;
>>> - expr_const_sets_destroy(&pg->port_groups);
>>> - shash_destroy(&pg->port_groups);
>>> +
>>> + expr_const_sets_destroy(&pg->port_groups_cs_local);
>>> + shash_destroy(&pg->port_groups_cs_local);
>>> +
>>> + port_group_ssets_clear(&pg->port_group_ssets);
>>> + shash_destroy(&pg->port_group_ssets);
>>> +
>>> sset_destroy(&pg->new);
>>> sset_destroy(&pg->deleted);
>>> sset_destroy(&pg->updated);
>>> }
>>>
>>> -/* Iterate port groups in the southbound database. Create and update
> the
>>> - * corresponding symtab entries as necessary. */
>>> - static void
>>> +static void
>>> port_groups_init(const struct sbrec_port_group_table *port_group_table,
>>> - struct shash *port_groups)
>>> + const struct sset *local_lports,
>>> + struct shash *port_group_ssets,
>>> + struct shash *port_groups_cs_local)
>>> {
>>> const struct sbrec_port_group *pg;
>>> SBREC_PORT_GROUP_TABLE_FOR_EACH (pg, port_group_table) {
>>> - expr_const_sets_add(port_groups, pg->name,
>>> + port_group_ssets_add_or_update(port_group_ssets, pg);
>>> + expr_const_sets_add(port_groups_cs_local, pg->name,
>>> (const char *const *) pg->ports,
>>> - pg->n_ports, false);
>>> + pg->n_ports, false, local_lports);
>>> }
>>> }
>>>
>>> static void
>>> port_groups_update(const struct sbrec_port_group_table
> *port_group_table,
>>> - struct shash *port_groups, struct sset *new,
>>> - struct sset *deleted, struct sset *updated)
>>> + const struct sset *local_lports,
>>> + struct shash *port_group_ssets,
>>> + struct shash *port_groups_cs_local,
>>> + struct sset *new, struct sset *deleted,
>>> + struct sset *updated)
>>> {
>>> const struct sbrec_port_group *pg;
>>> SBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (pg, port_group_table) {
>>> if (sbrec_port_group_is_deleted(pg)) {
>>> - expr_const_sets_remove(port_groups, pg->name);
>>> + expr_const_sets_remove(port_groups_cs_local, pg->name);
>>> + port_group_ssets_delete(port_group_ssets, pg->name);
>>> sset_add(deleted, pg->name);
>>> } else {
>>> - expr_const_sets_add(port_groups, pg->name,
>>> + expr_const_sets_add(port_groups_cs_local, pg->name,
>>> (const char *const *) pg->ports,
>>> - pg->n_ports, false);
>>> + pg->n_ports, false, local_lports);
>>> + port_group_ssets_add_or_update(port_group_ssets, pg);
>>
>> Bit of a nit: maybe the port_group_ssets_() suite of functions could
>> follow the same pattern as expr_const_sets_() by s/ssets/sets and
>> changing the verbs. It keeps it consistent and is basically working on
>> the same type of data sructure.
>>
>> e.g.
>>
>> port_group_sets_add()
>> port_group_sets_remove()
>> port_group_sets_destroy()
>>
>
> port_group_ssets is different from expr_const_sets. port_groups_ssets is a
> struct shash with each value item being a struct sset. expr_const_sets is
> also a struct shash but each item is an array of strings. I think different
> naming patterns would help remind that they are different.
>
>>> if (sbrec_port_group_is_new(pg)) {
>>> sset_add(new, pg->name);
>>> } else {
>>> @@ -1485,22 +1549,36 @@ port_groups_update(const struct
> sbrec_port_group_table *port_group_table,
>>> }
>>>
>>> static void
>>> -en_port_groups_run(struct engine_node *node, void *data)
>>> +en_port_groups_clear_tracked_data(void *data_)
>>> {
>>> - struct ed_type_port_groups *pg = data;
>>> -
>>> + struct ed_type_port_groups *pg = data_;
>>> sset_clear(&pg->new);
>>> sset_clear(&pg->deleted);
>>> sset_clear(&pg->updated);
>>> - expr_const_sets_destroy(&pg->port_groups);
>>> + pg->change_tracked = false;
>>> +}
>>> +
>>> +static void
>>> +en_port_groups_run(struct engine_node *node, void *data)
>>> +{
>>> + struct ed_type_port_groups *pg = data;
>>> +
>>> + expr_const_sets_destroy(&pg->port_groups_cs_local);
>>> + port_group_ssets_clear(&pg->port_group_ssets);
>>>
>>> struct sbrec_port_group_table *pg_table =
>>> (struct sbrec_port_group_table *)EN_OVSDB_GET(
>>> engine_get_input("SB_port_group", node));
>>>
>>> - port_groups_init(pg_table, &pg->port_groups);
>>> + struct ed_type_runtime_data *rt_data =
>>> + engine_get_input_data("runtime_data", node);
>>> +
>>> + struct sset *local_b_lports = binding_collect_local_binding_lports(
>>> + &rt_data->lbinding_data);
>>> + port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets,
>>> + &pg->port_groups_cs_local);
>>> + binding_destroy_local_binding_lports(local_b_lports);
>>>
>>> - pg->change_tracked = false;
>>> engine_set_node_state(node, EN_UPDATED);
>>> }
>>>
>>> @@ -1509,16 +1587,19 @@ port_groups_sb_port_group_handler(struct
> engine_node *node, void *data)
>>> {
>>> struct ed_type_port_groups *pg = data;
>>>
>>> - sset_clear(&pg->new);
>>> - sset_clear(&pg->deleted);
>>> - sset_clear(&pg->updated);
>>> -
>>> struct sbrec_port_group_table *pg_table =
>>> (struct sbrec_port_group_table *)EN_OVSDB_GET(
>>> engine_get_input("SB_port_group", node));
>>>
>>> - port_groups_update(pg_table, &pg->port_groups, &pg->new,
>>> - &pg->deleted, &pg->updated);
>>> + struct ed_type_runtime_data *rt_data =
>>> + engine_get_input_data("runtime_data", node);
>>> +
>>> + struct sset *local_b_lports = binding_collect_local_binding_lports(
>>> + &rt_data->lbinding_data);
>>> + port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets,
>>> + &pg->port_groups_cs_local, &pg->new,
> &pg->deleted,
>>> + &pg->updated);
>>> + binding_destroy_local_binding_lports(local_b_lports);
>>>
>>> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
>>> !sset_is_empty(&pg->updated)) {
>>> @@ -1531,6 +1612,74 @@ port_groups_sb_port_group_handler(struct
> engine_node *node, void *data)
>>> return true;
>>> }
>>>
>>> +static bool
>>> +port_groups_runtime_data_handler(struct engine_node *node, void *data)
>>> +{
>>> + struct ed_type_port_groups *pg = data;
>>> +
>>> + struct sbrec_port_group_table *pg_table =
>>> + (struct sbrec_port_group_table *)EN_OVSDB_GET(
>>> + engine_get_input("SB_port_group", node));
>>> +
>>> + struct ed_type_runtime_data *rt_data =
>>> + engine_get_input_data("runtime_data", node);
>>> +
>>> + if (!rt_data->tracked) {
>>> + return false;
>>> + }
>>> +
>>> + if (hmap_is_empty(&rt_data->tracked_dp_bindings)) {
>>> + goto out;
>>> + }
>>> +
>>> + struct sset *local_b_lports = binding_collect_local_binding_lports(
>>> + &rt_data->lbinding_data);
>>> +
>>> + const struct sbrec_port_group *pg_sb;
>>> + SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) {
>>> +
>> Remove blank line^
>
> Ack
>
>>> + struct sset *pg_lports = shash_find_data(&pg->port_group_ssets,
>>> + pg_sb->name);
>>
>> Correct me if I am wrong but I think this^ is the only reason you need
>> the 'port_group_ssets' data structure? Perhaps you could remove the need
>> for it by implementing expr_const_sets_find() function? It would
>> simplify the code a lot as you would only need 'port_groups_cs_local'.
>> If you could do this, you could also rename that to 'port_groups_local'
>
> In fact, what's more important of the usage of "port_group_ssets" is the
> check in the below loop:
>
> if (sset_contains(pg_lports, lport->pb->logical_port)) {
> ...
>
> It is to quickly check if a local port binding is affecting that PG.
> Without the "port_group_ssets" we will have to do a O(n) iteration of each
> lport of a PG. This is also briefly mentioned in the definition:
>
> /* A copy of SB port_groups, each converted as a sset for efficient
> lport
> * lookup. */
> struct shash port_group_ssets;
>
> It can't be replaced by 'expr_const_sets_find' because they contain
> different information (local lports v.s. all lports) and also different
> data structure (array v.s. sset).
Yes, I actually realized that when I was thinking about it again
yesterday evening. There are two things that were confusing me
yesterday: 1) there are now three layers of abstraction in this code
(en_port_groups_, port_groups_, and port_groups_ssets_). I get it after
looking at it for a few times but its not immediately clear because the
names are similar. 2) The loops in the function
(port_groups_runtime_data_handler) are quite complicated due to having 3
nesting levels. I dont think there is anything functionally wrong with
it but I was hoping to simplify it somehow and I thought we could remove
the 'port_groups_ssets' abstraction.
For 1) maybe alot of this code could be organised into a port_groups.c/h
file?
For 2)Why can we not just iterate over the port groups in the sb
database and call port_groups_update()? Is this an optimization?
>
>>> + ovs_assert(pg_lports);
>>> +
>>> + struct tracked_binding_datapath *tdp;
>>> + bool need_update = false;
>>> + HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>>> + struct shash_node *shash_node;
>>> + SHASH_FOR_EACH (shash_node, &tdp->lports) {
>>> + struct tracked_binding_lport *lport = shash_node->data;
>>> + if (sset_contains(pg_lports, lport->pb->logical_port))
> {
>>> + /* At least one local port-binding change is
> related to the
>>> + * port_group, so the port_group_cs_local needs
> update. */
>>> + need_update = true;
>>> + break;
>>> + }
>>> + }
>>> + if (need_update) {
>>> + break;
>>> + }
>>> + }
>>> + if (need_update) {
>>> + expr_const_sets_add(&pg->port_groups_cs_local, pg_sb->name,
>>> + (const char *const *) pg_sb->ports,
>>> + pg_sb->n_ports, false, local_b_lports);
>>> + sset_add(&pg->updated, pg_sb->name);
>>> + }
>>> + }
>>> +
>>> + binding_destroy_local_binding_lports(local_b_lports);
>>> +
>>> +out:
>>> + if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
>>> + !sset_is_empty(&pg->updated)) {
>>> + engine_set_node_state(node, EN_UPDATED);
>>> + } else {
>>> + engine_set_node_state(node, EN_UNCHANGED);
>>> + }
>>> + pg->change_tracked = true;
>>> + return true;
>>> +}
>>> +
>>> /* Connection tracking zones. */
>>> struct ed_type_ct_zones {
>>> unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
>>> @@ -1880,7 +2029,7 @@ static void init_lflow_ctx(struct engine_node
> *node,
>>>
>>> struct ed_type_port_groups *pg_data =
>>> engine_get_input_data("port_groups", node);
>>> - struct shash *port_groups = &pg_data->port_groups;
>>> + struct shash *port_groups = &pg_data->port_groups_cs_local;
>>>
>>> l_ctx_in->sbrec_multicast_group_by_name_datapath =
>>> sbrec_mc_group_by_name_dp;
>>> @@ -2540,7 +2689,7 @@ main(int argc, char *argv[])
>>> "physical_flow_changes");
>>> ENGINE_NODE(flow_output, "flow_output");
>>> ENGINE_NODE(addr_sets, "addr_sets");
>>> - ENGINE_NODE(port_groups, "port_groups");
>>> + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
>>>
>>> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
>>> SB_NODES
>>> @@ -2556,6 +2705,10 @@ main(int argc, char *argv[])
>>> addr_sets_sb_address_set_handler);
>>> engine_add_input(&en_port_groups, &en_sb_port_group,
>>> port_groups_sb_port_group_handler);
>>> + /* port_groups computation requires runtime_data's lbinding_data
> for the
>>> + * locally bound ports. */
>>> + engine_add_input(&en_port_groups, &en_runtime_data,
>>> + port_groups_runtime_data_handler);
>>>
>>> /* Engine node physical_flow_changes indicates whether
>>> * we can recompute only physical flows or we can
>>> @@ -2986,7 +3139,7 @@ main(int argc, char *argv[])
>>> engine_get_data(&en_port_groups);
>>> if (br_int && chassis && as_data && pg_data) {
>>> char *error = ofctrl_inject_pkt(br_int,
> pending_pkt.flow_s,
>>> - &as_data->addr_sets, &pg_data->port_groups);
>>> + &as_data->addr_sets,
> &pg_data->port_groups_cs_local);
>>> if (error) {
>>> unixctl_command_reply_error(pending_pkt.conn,
> error);
>>> free(error);
>>> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
>>> index 032370058..2f7ecbc9c 100644
>>> --- a/include/ovn/expr.h
>>> +++ b/include/ovn/expr.h
>>> @@ -547,7 +547,7 @@ void expr_constant_set_destroy(struct
> expr_constant_set *cs);
>>>
>>> void expr_const_sets_add(struct shash *const_sets, const char *name,
>>> const char * const *values, size_t n_values,
>>> - bool convert_to_integer);
>>> + bool convert_to_integer, const struct sset
> *filter);
>>> void expr_const_sets_remove(struct shash *const_sets, const char
> *name);
>>> void expr_const_sets_destroy(struct shash *const_sets);
>>>
>>> diff --git a/lib/expr.c b/lib/expr.c
>>> index f061a8fbe..b42d78d3e 100644
>>> --- a/lib/expr.c
>>> +++ b/lib/expr.c
>>> @@ -1065,7 +1065,7 @@ expr_constant_set_destroy(struct
> expr_constant_set *cs)
>>> void
>>
>>> expr_const_sets_add(struct shash *const_sets, const char *name,
>>> const char *const *values, size_t n_values,
>>> - bool convert_to_integer)
>>> + bool convert_to_integer, const struct sset *filter)
>>> {
>>> /* Replace any existing entry for this name. */
>>> expr_const_sets_remove(const_sets, name);
>>> @@ -1075,6 +1075,7 @@ expr_const_sets_add(struct shash *const_sets,
> const char *name,
>>> cs->n_values = 0;
>>> cs->values = xmalloc(n_values * sizeof *cs->values);
>>> if (convert_to_integer) {
>>> + ovs_assert(!filter);
>> Because the parameters of this function do not apply to both the integer
>> and the string case, this implies all the callers must know if they are
>> dealing with integers or strings. I'm wondering should this function be
>> split into something like:
>>
>> expr_const_sets_add_ints()
>> expr_const_sets_add_strings()
>>
>> and maybe even:
>>
>> expr_const_sets_add_filtered_strings()
>>
>> I just looked through the code and this seems to be the case - in fact
>> 'convert_to_integer' basically means that this is an integer. What do
>> you think?
>>
>
> Agree. I thought about the same thing but was lazy :). I can change it in
> the next version of the patch.
Great
>
>>> cs->type = EXPR_C_INTEGER;
>>> for (size_t i = 0; i < n_values; i++) {
>>> /* Use the lexer to convert each constant set into the
> proper
>>> @@ -1100,6 +1101,11 @@ expr_const_sets_add(struct shash *const_sets,
> const char *name,
>>> } else {
>>> cs->type = EXPR_C_STRING;
>>> for (size_t i = 0; i < n_values; i++) {
>>> + if (filter && !sset_find(filter, values[i])) {
>>> + VLOG_DBG("Skip constant set entry '%s' for '%s'",
>>> + values[i], name);
>>> + continue;
>>> + }
>>> union expr_constant *c = &cs->values[cs->n_values++];
>>> c->string = xstrdup(values[i]);
>>> }
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index 55444bbd7..aa1d1cf17 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -26265,3 +26265,56 @@ primary lport : [[sw0p2]]
>>> OVN_CLEANUP([hv1], [hv2])
>>> AT_CLEANUP
>>> ])
>>> +
>>> +# Tests the efficiency of conjunction flow generation when port groups
> are used
>>> +# by ACLs. Make sure there is no open flow explosion in table 45 for
> an ACL
>>> +# with self-referencing match condition of a port group:
>>> +#
>>> +# "outport == @pg1 && ip4.src == $pg1_ip4"
>>> +#
>>> +# 10 LSes (ls[0-9]), each has 2 LSPs (lsp[0-9][01]). All LSPs belong
> to port
>>> +# group pg1, but only LSPs of LS0 are bound on HV1.
>>> +#
>>> +# The expected number of conjunction flows is 2 + 20 = 22.
>>
>> A brief explanation of why that number is expected should help our
>> future selves.
>
> Ack
>
>>> +OVN_FOR_EACH_NORTHD([
>>> +AT_SETUP([ovn -- ACL with Port Group conjunction flow efficiency])
>>> +ovn_start
>>> +
>>> +net_add n1
>>> +
>>> +sim_add hv1
>>> +as hv1
>>
>> Maybe this test should be run with 2 hypervisors as it will catch any
>> corner cases that may come up due to the locality of ports and
>> datapaths? What do you think?
>>
>
> In fact we have at least another 2 test cases, "Port Group" and "ACLs on
> Port Groups", both have 3 HVs, so I think the coverage is good. I could
> have added the flow count check in existing cases but it seems more clear
> to add a new test case focusing on the flow count efficiency only, so I
> added this new test case and I think one HV is enough for this purpose.
> What do you think?
>
Ok, I had a look at those tests and this seems Ok. Your strategy makes
sense.
> Thanks again for all your comments. I will update with v3.
>
> Han
>
>>> +ovs-vsctl add-br br-phys
>>> +ovn_attach n1 br-phys 192.168.0.1
>>> +
>>> +ovn-nbctl lr-add lr0
>>> +
>>> +for i in $(seq 0 9); do
>>> + ovn-nbctl ls-add ls$i
>>> + ovn-nbctl lrp-add lr0 lrp_lr0_ls$i aa:bb:bb:00:00:0$i
> 192.168.${i}.1/24
>>> +
>>> + ovn-nbctl lsp-add ls$i lsp_ls${i}_lr0 -- \
>>> + lsp-set-addresses lsp_ls${i}_lr0 router -- \
>>> + lsp-set-type lsp_ls${i}_lr0 router -- \
>>> + lsp-set-options lsp_ls${i}_lr0 router-port=lrp_lr0_ls${i}
>>> +
>>> + for j in 0 1; do
>>> + ovn-nbctl lsp-add ls$i lsp${i}-${j} -- \
>>> + lsp-set-addresses lsp${i}-${j} "aa:aa:aa:00:0$i:0$j
> 192.168.$i.1$j"
>>> + done
>>> +done
>>> +
>>> +ovn-nbctl pg-add pg1
>>> +ovn-nbctl pg-set-ports pg1 $(for i in 0 1 2 3 4 5 6 7 8 9; do for j in
> 0 1; do echo lsp${i}-${j}; done; done)
>>> +
>>> +ovn-nbctl --type=port-group acl-add pg1 to-lport 100 "outport == @pg1
> && ip4.src == \$pg1_ip4" allow
>>> +
>>> +ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0
> external_ids:iface-id=lsp0-0
>>> +ovs-vsctl add-port br-int lsp0-1 -- set interface lsp0-1
> external_ids:iface-id=lsp0-1
>>> +
>>> +check ovn-nbctl --wait=hv sync
>>> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=45 | grep
> conjunction | wc -l) == 22])
>>> +
>>> +OVN_CLEANUP([hv1])
>>> +AT_CLEANUP
>>> +])
>>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>>> index 202a96c5d..55dbbb6b4 100644
>>> --- a/tests/test-ovn.c
>>> +++ b/tests/test-ovn.c
>>> @@ -227,10 +227,10 @@ create_addr_sets(struct shash *addr_sets)
>>> };
>>> static const char *const addrs4[] = { NULL };
>>>
>>> - expr_const_sets_add(addr_sets, "set1", addrs1, 3, true);
>>> - expr_const_sets_add(addr_sets, "set2", addrs2, 3, true);
>>> - expr_const_sets_add(addr_sets, "set3", addrs3, 3, true);
>>> - expr_const_sets_add(addr_sets, "set4", addrs4, 0, true);
>>> + expr_const_sets_add(addr_sets, "set1", addrs1, 3, true, NULL);
>>> + expr_const_sets_add(addr_sets, "set2", addrs2, 3, true, NULL);
>>> + expr_const_sets_add(addr_sets, "set3", addrs3, 3, true, NULL);
>>> + expr_const_sets_add(addr_sets, "set4", addrs4, 0, true, NULL);
>>> }
>>>
>>> static void
>>> @@ -243,8 +243,8 @@ create_port_groups(struct shash *port_groups)
>>> };
>>> static const char *const pg2[] = { NULL };
>>>
>>> - expr_const_sets_add(port_groups, "0_pg1", pg1, 3, false);
>>> - expr_const_sets_add(port_groups, "0_pg_empty", pg2, 0, false);
>>> + expr_const_sets_add(port_groups, "0_pg1", pg1, 3, false, NULL);
>>> + expr_const_sets_add(port_groups, "0_pg_empty", pg2, 0, false,
> NULL);
>>> }
>>>
>>> static bool
>>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>>> index 6b883886f..63b640db9 100644
>>> --- a/utilities/ovn-trace.c
>>> +++ b/utilities/ovn-trace.c
>>> @@ -820,7 +820,7 @@ read_address_sets(void)
>>> SBREC_ADDRESS_SET_FOR_EACH (sbas, ovnsb_idl) {
>>> expr_const_sets_add(&address_sets, sbas->name,
>>> (const char *const *) sbas->addresses,
>>> - sbas->n_addresses, true);
>>> + sbas->n_addresses, true, NULL);
>>> }
>>> }
>>>
>>> @@ -833,7 +833,7 @@ read_port_groups(void)
>>> SBREC_PORT_GROUP_FOR_EACH (sbpg, ovnsb_idl) {
>>> expr_const_sets_add(&port_groups, sbpg->name,
>>> (const char *const *) sbpg->ports,
>>> - sbpg->n_ports, false);
>>> + sbpg->n_ports, false, NULL);
>>> }
>>> }
>>>
>>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
