Message ID | 20220209175450.1179778-4-hzhou@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | ovn-controller: Fine-grained address set incremental processing. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Wed, Feb 9, 2022 at 12:55 PM Han Zhou <hzhou@ovn.org> wrote: > > This patch tracks individual address information when parsing address > sets from logical flows, and link to the corresponding desired flow > resulted from the IP. > > Signed-off-by: Han Zhou <hzhou@ovn.org> Hi Han, Please see below for a couple of small comments. Numan > --- > controller/lflow.c | 19 ++++-- > controller/ofctrl.c | 130 ++++++++++++++++++++++++++++++++++++++---- > controller/ofctrl.h | 19 ++++-- > controller/physical.c | 2 +- > include/ovn/expr.h | 9 +++ > lib/expr.c | 81 ++++++++++++++++++++------ > 6 files changed, 224 insertions(+), 36 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 8b32c7469..79652735c 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -692,13 +692,23 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > } > } > } > + > + struct addrset_info as_info = { > + .name = m->as_name, > + .ip = m->as_ip, > + .mask = m->as_mask > + }; > if (!m->n) { > ofctrl_add_flow_metered(l_ctx_out->flow_table, ptable, > lflow->priority, > lflow->header_.uuid.parts[0], &m->match, > &ofpacts, &lflow->header_.uuid, > - ctrl_meter_id); > + ctrl_meter_id, > + as_info.name ? &as_info : NULL); > } else { > + if (m->n > 1) { > + ovs_assert(!as_info.name); > + } > uint64_t conj_stubs[64 / 8]; > struct ofpbuf conj; > > @@ -716,7 +726,8 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable, > lflow->priority, 0, > &m->match, &conj, &lflow->header_.uuid, > - ctrl_meter_id); > + ctrl_meter_id, > + as_info.name ? &as_info : NULL); > ofpbuf_uninit(&conj); > } > } > @@ -1524,7 +1535,7 @@ add_lb_ct_snat_hairpin_dp_flows(struct ovn_controller_lb *lb, > ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, 200, > lb->slb->header_.uuid.parts[0], > &dp_match, &dp_acts, &lb->slb->header_.uuid, > - NX_CTLR_NO_METER); > + NX_CTLR_NO_METER, NULL); > } > > ofpbuf_uninit(&dp_acts); > @@ -1698,7 +1709,7 @@ add_lb_ct_snat_hairpin_vip_flow(struct ovn_controller_lb *lb, > ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, priority, > lb->slb->header_.uuid.parts[0], > &match, &ofpacts, &lb->slb->header_.uuid, > - NX_CTLR_NO_METER); > + NX_CTLR_NO_METER, NULL); > ofpbuf_uninit(&ofpacts); > > } > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index bcd6cea79..7671a3b7a 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -165,15 +165,35 @@ struct sb_to_flow { > struct uuid sb_uuid; > struct ovs_list flows; /* A list of struct sb_flow_ref nodes that > are referenced by the sb_uuid. */ > + struct ovs_list addrsets; /* A list of struct sb_addrset_ref. */ > }; > > struct sb_flow_ref { > struct ovs_list sb_list; /* List node in desired_flow.references. */ > - struct ovs_list flow_list; /* List node in sb_to_flow.desired_flows. */ > + struct ovs_list flow_list; /* List node in sb_to_flow.flows. */ > + struct ovs_list as_ip_flow_list; /* List node in ip_to_flow_node.flows. */ > struct desired_flow *flow; > struct uuid sb_uuid; > }; > > +struct sb_addrset_ref { > + struct ovs_list list_node; /* List node in sb_to_flow.addrsets. */ > + char *name; /* Name of the address set. */ > + struct hmap ip_to_flow_map; /* map from IPs in the address set to flows. > + Each node is struct ip_to_flow_node. */ > +}; > + > +struct ip_to_flow_node { I'd suggest to change the name of this struct to "as_ip_to_flow_node" to make it explicitly clear that the "ip" here refers to the address set ip. > + struct hmap_node hmap_node; /* Node in sb_addrset_ref.ip_to_flow_map. */ > + struct in6_addr as_ip; > + struct in6_addr as_mask; > + > + /* A list of struct sb_flow_ref. A single IP in an address set can be > + * used by multiple flows. e.g., in match: > + * ip.src == $as1 && ip.dst == $as1. */ > + struct ovs_list flows; > +}; > + > /* An installed flow, in static variable installed_lflows/installed_pflows. > * > * Installed flows are updated in ofctrl_put for maintaining the flow > @@ -1030,9 +1050,26 @@ sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) > return NULL; > } > > +static struct ip_to_flow_node * > +ip_to_flow_find(struct hmap *ip_to_flow_map, const struct in6_addr *as_ip, > + const struct in6_addr *as_mask) > +{ > + uint32_t hash = hash_bytes(as_ip, sizeof *as_ip, 0); > + > + struct ip_to_flow_node *itfn; > + HMAP_FOR_EACH_WITH_HASH (itfn, hmap_node, hash, ip_to_flow_map) { > + if (ipv6_addr_equals(&itfn->as_ip, as_ip) > + && ipv6_addr_equals(&itfn->as_mask, as_mask)) { > + return itfn; > + } > + } > + return NULL; > +} > + > static void > link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > - struct desired_flow *f, const struct uuid *sb_uuid) > + struct desired_flow *f, const struct uuid *sb_uuid, > + const struct addrset_info *as_info) > { > struct sb_flow_ref *sfr = xmalloc(sizeof *sfr); > mem_stats.sb_flow_ref_usage += sb_flow_ref_size(sfr); > @@ -1046,10 +1083,48 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > mem_stats.sb_flow_ref_usage += sb_to_flow_size(stf); > stf->sb_uuid = *sb_uuid; > ovs_list_init(&stf->flows); > + ovs_list_init(&stf->addrsets); > hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node, > uuid_hash(sb_uuid)); > } > ovs_list_insert(&stf->flows, &sfr->flow_list); > + > + if (!as_info) { > + ovs_list_init(&sfr->as_ip_flow_list); > + return; > + } > + > + /* link flow to address_set + ip */ > + struct sb_addrset_ref *sar; > + bool found = false; > + LIST_FOR_EACH (sar, list_node, &stf->addrsets) { > + if (!strcmp(sar->name, as_info->name)) { > + found = true; > + break; > + } > + } > + if (!found) { > + sar = xmalloc(sizeof *sar); > + mem_stats.sb_flow_ref_usage += sizeof *sar; > + sar->name = xstrdup(as_info->name); > + hmap_init(&sar->ip_to_flow_map); > + ovs_list_insert(&stf->addrsets, &sar->list_node); > + } > + > + struct ip_to_flow_node * itfn = ip_to_flow_find(&sar->ip_to_flow_map, > + &as_info->ip, > + &as_info->mask); > + if (!itfn) { > + itfn = xmalloc(sizeof *itfn); > + mem_stats.sb_flow_ref_usage += sizeof *itfn; > + itfn->as_ip = as_info->ip; > + itfn->as_mask = as_info->mask; > + ovs_list_init(&itfn->flows); > + uint32_t hash = hash_bytes(&as_info->ip, sizeof as_info->ip, 0); > + hmap_insert(&sar->ip_to_flow_map, &itfn->hmap_node, hash); > + } > + > + ovs_list_insert(&itfn->flows, &sfr->as_ip_flow_list); > } > > /* Flow table interfaces to the rest of ovn-controller. */ > @@ -1068,13 +1143,17 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > void > ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, > uint8_t table_id, uint16_t priority, > - uint64_t cookie, const struct match *match, > + uint64_t cookie, > + const struct match *match, > const struct ofpbuf *actions, > const struct uuid *sb_uuid, > - uint32_t meter_id, bool log_duplicate_flow) > + uint32_t meter_id, > + const struct addrset_info *as_info, > + bool log_duplicate_flow) > { > struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, > - match, actions, meter_id); > + match, actions, > + meter_id); > > if (desired_flow_lookup_check_uuid(flow_table, &f->flow, sb_uuid)) { > if (log_duplicate_flow) { > @@ -1091,7 +1170,7 @@ ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, > > hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, > f->flow.hash); > - link_flow_to_sb(flow_table, f, sb_uuid); > + link_flow_to_sb(flow_table, f, sb_uuid, as_info); > track_flow_add_or_modify(flow_table, f); > ovn_flow_log(&f->flow, "ofctrl_add_flow"); > } > @@ -1103,7 +1182,7 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows, > const struct uuid *sb_uuid) > { > ofctrl_add_flow_metered(desired_flows, table_id, priority, cookie, > - match, actions, sb_uuid, NX_CTLR_NO_METER); > + match, actions, sb_uuid, NX_CTLR_NO_METER, NULL); > } > > void > @@ -1111,11 +1190,12 @@ ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, > uint8_t table_id, uint16_t priority, uint64_t cookie, > const struct match *match, > const struct ofpbuf *actions, > - const struct uuid *sb_uuid, uint32_t meter_id) > + const struct uuid *sb_uuid, uint32_t meter_id, > + const struct addrset_info *as_info) > { > ofctrl_check_and_add_flow_metered(desired_flows, table_id, priority, > cookie, match, actions, sb_uuid, > - meter_id, true); > + meter_id, as_info, true); > } > > /* Either add a new flow, or append actions on an existing flow. If the > @@ -1127,7 +1207,8 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > const struct match *match, > const struct ofpbuf *actions, > const struct uuid *sb_uuid, > - uint32_t meter_id) > + uint32_t meter_id, > + const struct addrset_info *as_info) > { > struct desired_flow *existing; > struct desired_flow *f; > @@ -1156,11 +1237,20 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > ofpbuf_uninit(&compound); > desired_flow_destroy(f); > f = existing; > + > + /* Remove as_info tracking for the existing flow. */ > + struct sb_flow_ref *sfr; > + LIST_FOR_EACH (sfr, sb_list, &f->references) { > + ovs_list_remove(&sfr->as_ip_flow_list); > + ovs_list_init(&sfr->as_ip_flow_list); > + } > + /* Link to sb but don't track the as_info. */ Can you please add the comment to why the as_info is not tracked here ? From what I understand we don't track the as_info for flows with conjunction actions. Numan > + link_flow_to_sb(desired_flows, f, sb_uuid, NULL); > } else { > hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node, > f->flow.hash); > + link_flow_to_sb(desired_flows, f, sb_uuid, as_info); > } > - link_flow_to_sb(desired_flows, f, sb_uuid); > track_flow_add_or_modify(desired_flows, f); > > if (existing) { > @@ -1269,6 +1359,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { > ovs_list_remove(&sfr->sb_list); > ovs_list_remove(&sfr->flow_list); > + ovs_list_remove(&sfr->as_ip_flow_list); > struct desired_flow *f = sfr->flow; > mem_stats.sb_flow_ref_usage -= sb_flow_ref_size(sfr); > free(sfr); > @@ -1286,6 +1377,22 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > } > } > > + struct sb_addrset_ref *sar, *next_sar; > + LIST_FOR_EACH_SAFE (sar, next_sar, list_node, &stf->addrsets) { > + ovs_list_remove(&sar->list_node); > + struct ip_to_flow_node *itfn, *itfn_next; > + HMAP_FOR_EACH_SAFE (itfn, itfn_next, hmap_node, &sar->ip_to_flow_map) { > + hmap_remove(&sar->ip_to_flow_map, &itfn->hmap_node); > + ovs_assert(ovs_list_is_empty(&itfn->flows)); > + mem_stats.sb_flow_ref_usage -= sizeof *itfn; > + free(itfn); > + } > + hmap_destroy(&sar->ip_to_flow_map); > + mem_stats.sb_flow_ref_usage -= (sizeof *sar + strlen(sar->name) + 1); > + free(sar->name); > + free(sar); > + } > + > hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); > mem_stats.sb_flow_ref_usage -= sb_to_flow_size(stf); > free(stf); > @@ -1300,6 +1407,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > ovs_assert(!ovs_list_is_empty(&f->references)); > LIST_FOR_EACH (sfr, sb_list, &f->references) { > ovs_list_remove(&sfr->flow_list); > + ovs_list_remove(&sfr->as_ip_flow_list); > } > } > LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) { > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > index 014de210d..4ec328c24 100644 > --- a/controller/ofctrl.h > +++ b/controller/ofctrl.h > @@ -71,24 +71,34 @@ char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, > const struct shash *port_groups); > > /* Flow table interfaces to the rest of ovn-controller. */ > + > +/* Information of IP of an address set used to track a flow that is generated > + * from a logical flow referencing address set(s). */ > +struct addrset_info { > + const char *name; /* The address set's name. */ > + struct in6_addr ip; /* An IP in the address set. */ > + struct in6_addr mask; /* The mask of the IP. */ > +}; > void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id, > uint16_t priority, uint64_t cookie, > const struct match *, const struct ofpbuf *ofpacts, > const struct uuid *); > > -void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > +void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *, > uint8_t table_id, uint16_t priority, > - uint64_t cookie, const struct match *match, > + uint64_t cookie, const struct match *, > const struct ofpbuf *actions, > const struct uuid *sb_uuid, > - uint32_t meter_id); > + uint32_t meter_id, > + const struct addrset_info *); > > void ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, > uint8_t table_id, uint16_t priority, > uint64_t cookie, const struct match *match, > const struct ofpbuf *actions, > const struct uuid *sb_uuid, > - uint32_t meter_id); > + uint32_t meter_id, > + const struct addrset_info *); > > /* Removes a bundles of flows from the flow table for a specific sb_uuid. The > * flows are removed only if they are not referenced by any other sb_uuid(s). > @@ -123,6 +133,7 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, > uint64_t cookie, const struct match *, > const struct ofpbuf *ofpacts, > const struct uuid *, uint32_t meter_id, > + const struct addrset_info *, > bool log_duplicate_flow); > > > diff --git a/controller/physical.c b/controller/physical.c > index 6bfa2304d..033828d57 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -835,7 +835,7 @@ put_local_common_flows(uint32_t dp_key, > put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p)); > ofctrl_check_and_add_flow_metered(flow_table, OFTABLE_SAVE_INPORT, 100, > 0, &match, ofpacts_p, hc_uuid, > - NX_CTLR_NO_METER, false); > + NX_CTLR_NO_METER, NULL, false); > } > } > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > index 3b5653f7b..5572a1071 100644 > --- a/include/ovn/expr.h > +++ b/include/ovn/expr.h > @@ -367,6 +367,8 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); > struct expr { > struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ > enum expr_type type; /* Expression type. */ > + char *as_name; /* Address set name. Null if it is not an > + address set. */ > > union { > /* EXPR_T_CMP. > @@ -469,6 +471,11 @@ struct expr_match { > struct match match; > struct cls_conjunction *conjunctions; > size_t n, allocated; > + > + /* Tracked address set information. */ > + char *as_name; > + struct in6_addr as_ip; > + struct in6_addr as_mask; > }; > > uint32_t expr_to_matches(const struct expr *, > @@ -526,6 +533,8 @@ struct expr_constant_set { > size_t n_values; /* Number of constants. */ > enum expr_constant_type type; /* Type of the constants. */ > bool in_curlies; /* Whether the constants were in {}. */ > + char *as_name; /* Name of an address set. It is NULL if not > + an address set. */ > }; > > bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); > diff --git a/lib/expr.c b/lib/expr.c > index 5fc6c1ce9..af083190f 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -160,7 +160,7 @@ expr_relop_test(enum expr_relop relop, int cmp) > struct expr * > expr_create_andor(enum expr_type type) > { > - struct expr *e = xmalloc(sizeof *e); > + struct expr *e = xzalloc(sizeof *e); > e->type = type; > ovs_list_init(&e->andor); > return e; > @@ -190,9 +190,17 @@ expr_combine(enum expr_type type, struct expr *a, struct expr *b) > } else { > ovs_list_push_back(&a->andor, &b->node); > } > + if (a->as_name) { > + free(a->as_name); > + a->as_name = NULL; > + } > return a; > } else if (b->type == type) { > ovs_list_push_front(&b->andor, &a->node); > + if (b->as_name) { > + free(b->as_name); > + b->as_name = NULL; > + } > return b; > } else { > struct expr *e = expr_create_andor(type); > @@ -220,7 +228,7 @@ expr_insert_andor(struct expr *andor, struct expr *before, struct expr *new) > struct expr * > expr_create_boolean(bool b) > { > - struct expr *e = xmalloc(sizeof *e); > + struct expr *e = xzalloc(sizeof *e); > e->type = EXPR_T_BOOLEAN; > e->boolean = b; > return e; > @@ -680,6 +688,10 @@ make_cmp(struct expr_context *ctx, > e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, > e, make_cmp__(f, r, &cs->values[i])); > } > + /* Track address set */ > + if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) { > + e->as_name = xstrdup(cs->as_name); > + } > exit: > expr_constant_set_destroy(cs); > return e; > @@ -802,6 +814,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, > return false; > } > > + if (!cs->n_values) { > + cs->as_name = xstrdup(ctx->lexer->token.s); > + } > + > size_t n_values = cs->n_values + addr_sets->n_values; > if (n_values >= *allocated_values) { > cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); > @@ -875,6 +891,13 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > sizeof *cs->values); > } > > + if (cs->as_name) { > + /* Combining other values to the constant set that is tracking an > + * address set, so untrack it. */ > + free(cs->as_name); > + cs->as_name = NULL; > + } > + > if (ctx->lexer->token.type == LEX_T_STRING) { > if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { > return false; > @@ -1057,6 +1080,7 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > } > } > free(cs->values); > + free(cs->as_name); > } > } > > @@ -1244,6 +1268,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) > c.values = cst; > c.n_values = 1; > c.in_curlies = false; > + c.as_name = NULL; > return make_cmp(ctx, &f, EXPR_R_NE, &c); > } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { > return make_cmp(ctx, &f, r, &c); > @@ -1709,6 +1734,7 @@ expr_symtab_destroy(struct shash *symtab) > static struct expr * > expr_clone_cmp(struct expr *expr) > { > + ovs_assert(!expr->as_name); > struct expr *new = xmemdup(expr, sizeof *expr); > if (!new->cmp.symbol->width) { > new->cmp.string = xstrdup(new->cmp.string); > @@ -1732,6 +1758,7 @@ expr_clone_andor(struct expr *expr) > static struct expr * > expr_clone_condition(struct expr *expr) > { > + ovs_assert(!expr->as_name); > struct expr *new = xmemdup(expr, sizeof *expr); > new->cond.string = xstrdup(new->cond.string); > return new; > @@ -1767,6 +1794,8 @@ expr_destroy(struct expr *expr) > return; > } > > + free(expr->as_name); > + > struct expr *sub, *next; > > switch (expr->type) { > @@ -2373,7 +2402,7 @@ crush_and_string(struct expr *expr, const struct expr_symbol *symbol) > > const char *string; > SSET_FOR_EACH (string, &result) { > - sub = xmalloc(sizeof *sub); > + sub = xzalloc(sizeof *sub); > sub->type = EXPR_T_CMP; > sub->cmp.relop = EXPR_R_EQ; > sub->cmp.symbol = symbol; > @@ -2432,7 +2461,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > return expr_create_boolean(true); > } else { > struct expr *cmp; > - cmp = xmalloc(sizeof *cmp); > + cmp = xzalloc(sizeof *cmp); > cmp->type = EXPR_T_CMP; > cmp->cmp.symbol = symbol; > cmp->cmp.relop = EXPR_R_EQ; > @@ -2447,7 +2476,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > struct expr *disjuncts = expr_from_node(ovs_list_pop_front(&expr->andor)); > struct expr *or; > > - or = xmalloc(sizeof *or); > + or = xzalloc(sizeof *or); > or->type = EXPR_T_OR; > ovs_list_init(&or->andor); > > @@ -2483,7 +2512,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > struct expr *new = NULL; > struct expr *or; > > - or = xmalloc(sizeof *or); > + or = xzalloc(sizeof *or); > or->type = EXPR_T_OR; > ovs_list_init(&or->andor); > > @@ -2502,7 +2531,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > LIST_FOR_EACH (b, node, &bs->andor) { > ovs_assert(b->type == EXPR_T_CMP); > if (!new) { > - new = xmalloc(sizeof *new); > + new = xzalloc(sizeof *new); > new->type = EXPR_T_CMP; > new->cmp.symbol = symbol; > new->cmp.relop = EXPR_R_EQ; > @@ -2608,6 +2637,9 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol) > ovs_list_push_back(&expr->andor, &b->node); > } else { > expr_destroy(b); > + /* Member modified, so untrack address set. */ > + free(expr->as_name); > + expr->as_name = NULL; > } > } > free(subs); > @@ -2659,7 +2691,7 @@ expr_sort(struct expr *expr) > ovs_assert(expr->type == EXPR_T_AND); > > size_t n = ovs_list_size(&expr->andor); > - struct expr_sort *subs = xmalloc(n * sizeof *subs); > + struct expr_sort *subs = xzalloc(n * sizeof *subs); > struct expr *sub; > size_t i; > > @@ -2884,7 +2916,7 @@ static struct expr_match * > expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, > uint32_t conj_id) > { > - struct expr_match *match = xmalloc(sizeof *match); > + struct expr_match *match = xzalloc(sizeof *match); > if (m) { > match->match = *m; > } else { > @@ -2905,6 +2937,14 @@ expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, > return match; > } > > +static void > +expr_match_destroy(struct expr_match *match) > +{ > + free(match->as_name); > + free(match->conjunctions); > + free(match); > +} > + > /* Adds 'match' to hash table 'matches', which becomes the new owner of > * 'match'. > * > @@ -2932,8 +2972,12 @@ expr_match_add(struct hmap *matches, struct expr_match *match) > } > m->conjunctions[m->n++] = match->conjunctions[0]; > } > - free(match->conjunctions); > - free(match); > + if (m->as_name) { > + /* m is combined with match. so untracked the address set. */ > + free(m->as_name); > + m->as_name = NULL; > + } > + expr_match_destroy(match); > return; > } > } > @@ -2994,12 +3038,18 @@ add_disjunction(const struct expr *or, > LIST_FOR_EACH (sub, node, &or->andor) { > struct expr_match *match = expr_match_new(m, clause, n_clauses, > conj_id); > + if (or->as_name) { > + ovs_assert(sub->type == EXPR_T_CMP); > + ovs_assert(sub->cmp.symbol->width); > + match->as_name = xstrdup(or->as_name); > + match->as_ip = sub->cmp.value.ipv6; > + match->as_mask = sub->cmp.mask.ipv6; > + } > if (constrain_match(sub, lookup_port, aux, &match->match)) { > expr_match_add(matches, match); > n++; > } else { > - free(match->conjunctions); > - free(match); > + expr_match_destroy(match); > } > } > > @@ -3082,7 +3132,7 @@ add_cmp_flow(const struct expr *cmp, > if (constrain_match(cmp, lookup_port, aux, &m->match)) { > expr_match_add(matches, m); > } else { > - free(m); > + expr_match_destroy(m); > } > } > > @@ -3214,8 +3264,7 @@ expr_matches_destroy(struct hmap *matches) > } > > HMAP_FOR_EACH_POP (m, hmap_node, matches) { > - free(m->conjunctions); > - free(m); > + expr_match_destroy(m); > } > hmap_destroy(matches); > } > -- > 2.30.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Feb 23, 2022 at 2:50 PM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Feb 9, 2022 at 12:55 PM Han Zhou <hzhou@ovn.org> wrote: > > > > This patch tracks individual address information when parsing address > > sets from logical flows, and link to the corresponding desired flow > > resulted from the IP. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > Hi Han, > > Please see below for a couple of small comments. > > Numan > > > --- > > controller/lflow.c | 19 ++++-- > > controller/ofctrl.c | 130 ++++++++++++++++++++++++++++++++++++++---- > > controller/ofctrl.h | 19 ++++-- > > controller/physical.c | 2 +- > > include/ovn/expr.h | 9 +++ > > lib/expr.c | 81 ++++++++++++++++++++------ > > 6 files changed, 224 insertions(+), 36 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 8b32c7469..79652735c 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -692,13 +692,23 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > > } > > } > > } > > + > > + struct addrset_info as_info = { > > + .name = m->as_name, > > + .ip = m->as_ip, > > + .mask = m->as_mask > > + }; > > if (!m->n) { > > ofctrl_add_flow_metered(l_ctx_out->flow_table, ptable, > > lflow->priority, > > lflow->header_.uuid.parts[0], &m->match, > > &ofpacts, &lflow->header_.uuid, > > - ctrl_meter_id); > > + ctrl_meter_id, > > + as_info.name ? &as_info : NULL); > > } else { > > + if (m->n > 1) { > > + ovs_assert(!as_info.name); > > + } > > uint64_t conj_stubs[64 / 8]; > > struct ofpbuf conj; > > > > @@ -716,7 +726,8 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > > ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable, > > lflow->priority, 0, > > &m->match, &conj, &lflow->header_.uuid, > > - ctrl_meter_id); > > + ctrl_meter_id, > > + as_info.name ? &as_info : NULL); > > ofpbuf_uninit(&conj); > > } > > } > > @@ -1524,7 +1535,7 @@ add_lb_ct_snat_hairpin_dp_flows(struct ovn_controller_lb *lb, > > ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, 200, > > lb->slb->header_.uuid.parts[0], > > &dp_match, &dp_acts, &lb->slb->header_.uuid, > > - NX_CTLR_NO_METER); > > + NX_CTLR_NO_METER, NULL); > > } > > > > ofpbuf_uninit(&dp_acts); > > @@ -1698,7 +1709,7 @@ add_lb_ct_snat_hairpin_vip_flow(struct ovn_controller_lb *lb, > > ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, priority, > > lb->slb->header_.uuid.parts[0], > > &match, &ofpacts, &lb->slb->header_.uuid, > > - NX_CTLR_NO_METER); > > + NX_CTLR_NO_METER, NULL); > > ofpbuf_uninit(&ofpacts); > > > > } > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index bcd6cea79..7671a3b7a 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -165,15 +165,35 @@ struct sb_to_flow { > > struct uuid sb_uuid; > > struct ovs_list flows; /* A list of struct sb_flow_ref nodes that > > are referenced by the sb_uuid. */ > > + struct ovs_list addrsets; /* A list of struct sb_addrset_ref. */ > > }; > > > > struct sb_flow_ref { > > struct ovs_list sb_list; /* List node in desired_flow.references. */ > > - struct ovs_list flow_list; /* List node in sb_to_flow.desired_flows. */ > > + struct ovs_list flow_list; /* List node in sb_to_flow.flows. */ > > + struct ovs_list as_ip_flow_list; /* List node in ip_to_flow_node.flows. */ > > struct desired_flow *flow; > > struct uuid sb_uuid; > > }; > > > > +struct sb_addrset_ref { > > + struct ovs_list list_node; /* List node in sb_to_flow.addrsets. */ > > + char *name; /* Name of the address set. */ > > + struct hmap ip_to_flow_map; /* map from IPs in the address set to flows. > > + Each node is struct ip_to_flow_node. */ > > +}; > > + > > +struct ip_to_flow_node { > > I'd suggest to change the name of this struct to "as_ip_to_flow_node" > to make it explicitly clear that > the "ip" here refers to the address set ip. > Ack. I did s/ip_to_flow/as_ip_to_flow for all related namings. > > > + struct hmap_node hmap_node; /* Node in sb_addrset_ref.ip_to_flow_map. */ > > + struct in6_addr as_ip; > > + struct in6_addr as_mask; > > + > > + /* A list of struct sb_flow_ref. A single IP in an address set can be > > + * used by multiple flows. e.g., in match: > > + * ip.src == $as1 && ip.dst == $as1. */ > > + struct ovs_list flows; > > +}; > > + > > /* An installed flow, in static variable installed_lflows/installed_pflows. > > * > > * Installed flows are updated in ofctrl_put for maintaining the flow > > @@ -1030,9 +1050,26 @@ sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) > > return NULL; > > } > > > > +static struct ip_to_flow_node * > > +ip_to_flow_find(struct hmap *ip_to_flow_map, const struct in6_addr *as_ip, > > + const struct in6_addr *as_mask) > > +{ > > + uint32_t hash = hash_bytes(as_ip, sizeof *as_ip, 0); > > + > > + struct ip_to_flow_node *itfn; > > + HMAP_FOR_EACH_WITH_HASH (itfn, hmap_node, hash, ip_to_flow_map) { > > + if (ipv6_addr_equals(&itfn->as_ip, as_ip) > > + && ipv6_addr_equals(&itfn->as_mask, as_mask)) { > > + return itfn; > > + } > > + } > > + return NULL; > > +} > > + > > static void > > link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > > - struct desired_flow *f, const struct uuid *sb_uuid) > > + struct desired_flow *f, const struct uuid *sb_uuid, > > + const struct addrset_info *as_info) > > { > > struct sb_flow_ref *sfr = xmalloc(sizeof *sfr); > > mem_stats.sb_flow_ref_usage += sb_flow_ref_size(sfr); > > @@ -1046,10 +1083,48 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > > mem_stats.sb_flow_ref_usage += sb_to_flow_size(stf); > > stf->sb_uuid = *sb_uuid; > > ovs_list_init(&stf->flows); > > + ovs_list_init(&stf->addrsets); > > hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node, > > uuid_hash(sb_uuid)); > > } > > ovs_list_insert(&stf->flows, &sfr->flow_list); > > + > > + if (!as_info) { > > + ovs_list_init(&sfr->as_ip_flow_list); > > + return; > > + } > > + > > + /* link flow to address_set + ip */ > > + struct sb_addrset_ref *sar; > > + bool found = false; > > + LIST_FOR_EACH (sar, list_node, &stf->addrsets) { > > + if (!strcmp(sar->name, as_info->name)) { > > + found = true; > > + break; > > + } > > + } > > + if (!found) { > > + sar = xmalloc(sizeof *sar); > > + mem_stats.sb_flow_ref_usage += sizeof *sar; > > + sar->name = xstrdup(as_info->name); > > + hmap_init(&sar->ip_to_flow_map); > > + ovs_list_insert(&stf->addrsets, &sar->list_node); > > + } > > + > > + struct ip_to_flow_node * itfn = ip_to_flow_find(&sar->ip_to_flow_map, > > + &as_info->ip, > > + &as_info->mask); > > + if (!itfn) { > > + itfn = xmalloc(sizeof *itfn); > > + mem_stats.sb_flow_ref_usage += sizeof *itfn; > > + itfn->as_ip = as_info->ip; > > + itfn->as_mask = as_info->mask; > > + ovs_list_init(&itfn->flows); > > + uint32_t hash = hash_bytes(&as_info->ip, sizeof as_info->ip, 0); > > + hmap_insert(&sar->ip_to_flow_map, &itfn->hmap_node, hash); > > + } > > + > > + ovs_list_insert(&itfn->flows, &sfr->as_ip_flow_list); > > } > > > > /* Flow table interfaces to the rest of ovn-controller. */ > > @@ -1068,13 +1143,17 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, > > void > > ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, > > uint8_t table_id, uint16_t priority, > > - uint64_t cookie, const struct match *match, > > + uint64_t cookie, > > + const struct match *match, > > const struct ofpbuf *actions, > > const struct uuid *sb_uuid, > > - uint32_t meter_id, bool log_duplicate_flow) > > + uint32_t meter_id, > > + const struct addrset_info *as_info, > > + bool log_duplicate_flow) > > { > > struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, > > - match, actions, meter_id); > > + match, actions, > > + meter_id); > > > > if (desired_flow_lookup_check_uuid(flow_table, &f->flow, sb_uuid)) { > > if (log_duplicate_flow) { > > @@ -1091,7 +1170,7 @@ ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, > > > > hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, > > f->flow.hash); > > - link_flow_to_sb(flow_table, f, sb_uuid); > > + link_flow_to_sb(flow_table, f, sb_uuid, as_info); > > track_flow_add_or_modify(flow_table, f); > > ovn_flow_log(&f->flow, "ofctrl_add_flow"); > > } > > @@ -1103,7 +1182,7 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows, > > const struct uuid *sb_uuid) > > { > > ofctrl_add_flow_metered(desired_flows, table_id, priority, cookie, > > - match, actions, sb_uuid, NX_CTLR_NO_METER); > > + match, actions, sb_uuid, NX_CTLR_NO_METER, NULL); > > } > > > > void > > @@ -1111,11 +1190,12 @@ ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, > > uint8_t table_id, uint16_t priority, uint64_t cookie, > > const struct match *match, > > const struct ofpbuf *actions, > > - const struct uuid *sb_uuid, uint32_t meter_id) > > + const struct uuid *sb_uuid, uint32_t meter_id, > > + const struct addrset_info *as_info) > > { > > ofctrl_check_and_add_flow_metered(desired_flows, table_id, priority, > > cookie, match, actions, sb_uuid, > > - meter_id, true); > > + meter_id, as_info, true); > > } > > > > /* Either add a new flow, or append actions on an existing flow. If the > > @@ -1127,7 +1207,8 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > > const struct match *match, > > const struct ofpbuf *actions, > > const struct uuid *sb_uuid, > > - uint32_t meter_id) > > + uint32_t meter_id, > > + const struct addrset_info *as_info) > > { > > struct desired_flow *existing; > > struct desired_flow *f; > > @@ -1156,11 +1237,20 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > > ofpbuf_uninit(&compound); > > desired_flow_destroy(f); > > f = existing; > > + > > + /* Remove as_info tracking for the existing flow. */ > > + struct sb_flow_ref *sfr; > > + LIST_FOR_EACH (sfr, sb_list, &f->references) { > > + ovs_list_remove(&sfr->as_ip_flow_list); > > + ovs_list_init(&sfr->as_ip_flow_list); > > + } > > + /* Link to sb but don't track the as_info. */ > > Can you please add the comment to why the as_info is not tracked here ? > From what I understand we don't track the as_info for flows with > conjunction actions. > > Numan > This is the case when an existing flow is updated with combined conjunctions. We don't track it because the single OVS flow is mapped to multiple lflows and handling deletion would be more complex. It may be further improved if needed, but it is not handled for now. This happens only with combined conjunctions. For *regular* conjunctions we still track it. I updated the comment as below: /* Since the flow now shared by more than one SB lflows, don't track * it with address set ips. So remove any existed as_info tracking, and * then add the new sb link without as_info. * * XXX: this may still be tracked if the flow is shared by different * lflows, but we need to remove the related conjunction from the * actions properly when handle addrset ip deletion, instead of simply * delete the flow. */ Thanks, Han > > > + link_flow_to_sb(desired_flows, f, sb_uuid, NULL); > > } else { > > hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node, > > f->flow.hash); > > + link_flow_to_sb(desired_flows, f, sb_uuid, as_info); > > } > > - link_flow_to_sb(desired_flows, f, sb_uuid); > > track_flow_add_or_modify(desired_flows, f); > > > > if (existing) { > > @@ -1269,6 +1359,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > > LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { > > ovs_list_remove(&sfr->sb_list); > > ovs_list_remove(&sfr->flow_list); > > + ovs_list_remove(&sfr->as_ip_flow_list); > > struct desired_flow *f = sfr->flow; > > mem_stats.sb_flow_ref_usage -= sb_flow_ref_size(sfr); > > free(sfr); > > @@ -1286,6 +1377,22 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > > } > > } > > > > + struct sb_addrset_ref *sar, *next_sar; > > + LIST_FOR_EACH_SAFE (sar, next_sar, list_node, &stf->addrsets) { > > + ovs_list_remove(&sar->list_node); > > + struct ip_to_flow_node *itfn, *itfn_next; > > + HMAP_FOR_EACH_SAFE (itfn, itfn_next, hmap_node, &sar->ip_to_flow_map) { > > + hmap_remove(&sar->ip_to_flow_map, &itfn->hmap_node); > > + ovs_assert(ovs_list_is_empty(&itfn->flows)); > > + mem_stats.sb_flow_ref_usage -= sizeof *itfn; > > + free(itfn); > > + } > > + hmap_destroy(&sar->ip_to_flow_map); > > + mem_stats.sb_flow_ref_usage -= (sizeof *sar + strlen(sar->name) + 1); > > + free(sar->name); > > + free(sar); > > + } > > + > > hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); > > mem_stats.sb_flow_ref_usage -= sb_to_flow_size(stf); > > free(stf); > > @@ -1300,6 +1407,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, > > ovs_assert(!ovs_list_is_empty(&f->references)); > > LIST_FOR_EACH (sfr, sb_list, &f->references) { > > ovs_list_remove(&sfr->flow_list); > > + ovs_list_remove(&sfr->as_ip_flow_list); > > } > > } > > LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) { > > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > > index 014de210d..4ec328c24 100644 > > --- a/controller/ofctrl.h > > +++ b/controller/ofctrl.h > > @@ -71,24 +71,34 @@ char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, > > const struct shash *port_groups); > > > > /* Flow table interfaces to the rest of ovn-controller. */ > > + > > +/* Information of IP of an address set used to track a flow that is generated > > + * from a logical flow referencing address set(s). */ > > +struct addrset_info { > > + const char *name; /* The address set's name. */ > > + struct in6_addr ip; /* An IP in the address set. */ > > + struct in6_addr mask; /* The mask of the IP. */ > > +}; > > void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id, > > uint16_t priority, uint64_t cookie, > > const struct match *, const struct ofpbuf *ofpacts, > > const struct uuid *); > > > > -void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, > > +void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *, > > uint8_t table_id, uint16_t priority, > > - uint64_t cookie, const struct match *match, > > + uint64_t cookie, const struct match *, > > const struct ofpbuf *actions, > > const struct uuid *sb_uuid, > > - uint32_t meter_id); > > + uint32_t meter_id, > > + const struct addrset_info *); > > > > void ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, > > uint8_t table_id, uint16_t priority, > > uint64_t cookie, const struct match *match, > > const struct ofpbuf *actions, > > const struct uuid *sb_uuid, > > - uint32_t meter_id); > > + uint32_t meter_id, > > + const struct addrset_info *); > > > > /* Removes a bundles of flows from the flow table for a specific sb_uuid. The > > * flows are removed only if they are not referenced by any other sb_uuid(s). > > @@ -123,6 +133,7 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, > > uint64_t cookie, const struct match *, > > const struct ofpbuf *ofpacts, > > const struct uuid *, uint32_t meter_id, > > + const struct addrset_info *, > > bool log_duplicate_flow); > > > > > > diff --git a/controller/physical.c b/controller/physical.c > > index 6bfa2304d..033828d57 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -835,7 +835,7 @@ put_local_common_flows(uint32_t dp_key, > > put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p)); > > ofctrl_check_and_add_flow_metered(flow_table, OFTABLE_SAVE_INPORT, 100, > > 0, &match, ofpacts_p, hc_uuid, > > - NX_CTLR_NO_METER, false); > > + NX_CTLR_NO_METER, NULL, false); > > } > > } > > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > > index 3b5653f7b..5572a1071 100644 > > --- a/include/ovn/expr.h > > +++ b/include/ovn/expr.h > > @@ -367,6 +367,8 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); > > struct expr { > > struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ > > enum expr_type type; /* Expression type. */ > > + char *as_name; /* Address set name. Null if it is not an > > + address set. */ > > > > union { > > /* EXPR_T_CMP. > > @@ -469,6 +471,11 @@ struct expr_match { > > struct match match; > > struct cls_conjunction *conjunctions; > > size_t n, allocated; > > + > > + /* Tracked address set information. */ > > + char *as_name; > > + struct in6_addr as_ip; > > + struct in6_addr as_mask; > > }; > > > > uint32_t expr_to_matches(const struct expr *, > > @@ -526,6 +533,8 @@ struct expr_constant_set { > > size_t n_values; /* Number of constants. */ > > enum expr_constant_type type; /* Type of the constants. */ > > bool in_curlies; /* Whether the constants were in {}. */ > > + char *as_name; /* Name of an address set. It is NULL if not > > + an address set. */ > > }; > > > > bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); > > diff --git a/lib/expr.c b/lib/expr.c > > index 5fc6c1ce9..af083190f 100644 > > --- a/lib/expr.c > > +++ b/lib/expr.c > > @@ -160,7 +160,7 @@ expr_relop_test(enum expr_relop relop, int cmp) > > struct expr * > > expr_create_andor(enum expr_type type) > > { > > - struct expr *e = xmalloc(sizeof *e); > > + struct expr *e = xzalloc(sizeof *e); > > e->type = type; > > ovs_list_init(&e->andor); > > return e; > > @@ -190,9 +190,17 @@ expr_combine(enum expr_type type, struct expr *a, struct expr *b) > > } else { > > ovs_list_push_back(&a->andor, &b->node); > > } > > + if (a->as_name) { > > + free(a->as_name); > > + a->as_name = NULL; > > + } > > return a; > > } else if (b->type == type) { > > ovs_list_push_front(&b->andor, &a->node); > > + if (b->as_name) { > > + free(b->as_name); > > + b->as_name = NULL; > > + } > > return b; > > } else { > > struct expr *e = expr_create_andor(type); > > @@ -220,7 +228,7 @@ expr_insert_andor(struct expr *andor, struct expr *before, struct expr *new) > > struct expr * > > expr_create_boolean(bool b) > > { > > - struct expr *e = xmalloc(sizeof *e); > > + struct expr *e = xzalloc(sizeof *e); > > e->type = EXPR_T_BOOLEAN; > > e->boolean = b; > > return e; > > @@ -680,6 +688,10 @@ make_cmp(struct expr_context *ctx, > > e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, > > e, make_cmp__(f, r, &cs->values[i])); > > } > > + /* Track address set */ > > + if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) { > > + e->as_name = xstrdup(cs->as_name); > > + } > > exit: > > expr_constant_set_destroy(cs); > > return e; > > @@ -802,6 +814,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, > > return false; > > } > > > > + if (!cs->n_values) { > > + cs->as_name = xstrdup(ctx->lexer->token.s); > > + } > > + > > size_t n_values = cs->n_values + addr_sets->n_values; > > if (n_values >= *allocated_values) { > > cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); > > @@ -875,6 +891,13 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > > sizeof *cs->values); > > } > > > > + if (cs->as_name) { > > + /* Combining other values to the constant set that is tracking an > > + * address set, so untrack it. */ > > + free(cs->as_name); > > + cs->as_name = NULL; > > + } > > + > > if (ctx->lexer->token.type == LEX_T_STRING) { > > if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { > > return false; > > @@ -1057,6 +1080,7 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > > } > > } > > free(cs->values); > > + free(cs->as_name); > > } > > } > > > > @@ -1244,6 +1268,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) > > c.values = cst; > > c.n_values = 1; > > c.in_curlies = false; > > + c.as_name = NULL; > > return make_cmp(ctx, &f, EXPR_R_NE, &c); > > } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { > > return make_cmp(ctx, &f, r, &c); > > @@ -1709,6 +1734,7 @@ expr_symtab_destroy(struct shash *symtab) > > static struct expr * > > expr_clone_cmp(struct expr *expr) > > { > > + ovs_assert(!expr->as_name); > > struct expr *new = xmemdup(expr, sizeof *expr); > > if (!new->cmp.symbol->width) { > > new->cmp.string = xstrdup(new->cmp.string); > > @@ -1732,6 +1758,7 @@ expr_clone_andor(struct expr *expr) > > static struct expr * > > expr_clone_condition(struct expr *expr) > > { > > + ovs_assert(!expr->as_name); > > struct expr *new = xmemdup(expr, sizeof *expr); > > new->cond.string = xstrdup(new->cond.string); > > return new; > > @@ -1767,6 +1794,8 @@ expr_destroy(struct expr *expr) > > return; > > } > > > > + free(expr->as_name); > > + > > struct expr *sub, *next; > > > > switch (expr->type) { > > @@ -2373,7 +2402,7 @@ crush_and_string(struct expr *expr, const struct expr_symbol *symbol) > > > > const char *string; > > SSET_FOR_EACH (string, &result) { > > - sub = xmalloc(sizeof *sub); > > + sub = xzalloc(sizeof *sub); > > sub->type = EXPR_T_CMP; > > sub->cmp.relop = EXPR_R_EQ; > > sub->cmp.symbol = symbol; > > @@ -2432,7 +2461,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > > return expr_create_boolean(true); > > } else { > > struct expr *cmp; > > - cmp = xmalloc(sizeof *cmp); > > + cmp = xzalloc(sizeof *cmp); > > cmp->type = EXPR_T_CMP; > > cmp->cmp.symbol = symbol; > > cmp->cmp.relop = EXPR_R_EQ; > > @@ -2447,7 +2476,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > > struct expr *disjuncts = expr_from_node(ovs_list_pop_front(&expr->andor)); > > struct expr *or; > > > > - or = xmalloc(sizeof *or); > > + or = xzalloc(sizeof *or); > > or->type = EXPR_T_OR; > > ovs_list_init(&or->andor); > > > > @@ -2483,7 +2512,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > > struct expr *new = NULL; > > struct expr *or; > > > > - or = xmalloc(sizeof *or); > > + or = xzalloc(sizeof *or); > > or->type = EXPR_T_OR; > > ovs_list_init(&or->andor); > > > > @@ -2502,7 +2531,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) > > LIST_FOR_EACH (b, node, &bs->andor) { > > ovs_assert(b->type == EXPR_T_CMP); > > if (!new) { > > - new = xmalloc(sizeof *new); > > + new = xzalloc(sizeof *new); > > new->type = EXPR_T_CMP; > > new->cmp.symbol = symbol; > > new->cmp.relop = EXPR_R_EQ; > > @@ -2608,6 +2637,9 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol) > > ovs_list_push_back(&expr->andor, &b->node); > > } else { > > expr_destroy(b); > > + /* Member modified, so untrack address set. */ > > + free(expr->as_name); > > + expr->as_name = NULL; > > } > > } > > free(subs); > > @@ -2659,7 +2691,7 @@ expr_sort(struct expr *expr) > > ovs_assert(expr->type == EXPR_T_AND); > > > > size_t n = ovs_list_size(&expr->andor); > > - struct expr_sort *subs = xmalloc(n * sizeof *subs); > > + struct expr_sort *subs = xzalloc(n * sizeof *subs); > > struct expr *sub; > > size_t i; > > > > @@ -2884,7 +2916,7 @@ static struct expr_match * > > expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, > > uint32_t conj_id) > > { > > - struct expr_match *match = xmalloc(sizeof *match); > > + struct expr_match *match = xzalloc(sizeof *match); > > if (m) { > > match->match = *m; > > } else { > > @@ -2905,6 +2937,14 @@ expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, > > return match; > > } > > > > +static void > > +expr_match_destroy(struct expr_match *match) > > +{ > > + free(match->as_name); > > + free(match->conjunctions); > > + free(match); > > +} > > + > > /* Adds 'match' to hash table 'matches', which becomes the new owner of > > * 'match'. > > * > > @@ -2932,8 +2972,12 @@ expr_match_add(struct hmap *matches, struct expr_match *match) > > } > > m->conjunctions[m->n++] = match->conjunctions[0]; > > } > > - free(match->conjunctions); > > - free(match); > > + if (m->as_name) { > > + /* m is combined with match. so untracked the address set. */ > > + free(m->as_name); > > + m->as_name = NULL; > > + } > > + expr_match_destroy(match); > > return; > > } > > } > > @@ -2994,12 +3038,18 @@ add_disjunction(const struct expr *or, > > LIST_FOR_EACH (sub, node, &or->andor) { > > struct expr_match *match = expr_match_new(m, clause, n_clauses, > > conj_id); > > + if (or->as_name) { > > + ovs_assert(sub->type == EXPR_T_CMP); > > + ovs_assert(sub->cmp.symbol->width); > > + match->as_name = xstrdup(or->as_name); > > + match->as_ip = sub->cmp.value.ipv6; > > + match->as_mask = sub->cmp.mask.ipv6; > > + } > > if (constrain_match(sub, lookup_port, aux, &match->match)) { > > expr_match_add(matches, match); > > n++; > > } else { > > - free(match->conjunctions); > > - free(match); > > + expr_match_destroy(match); > > } > > } > > > > @@ -3082,7 +3132,7 @@ add_cmp_flow(const struct expr *cmp, > > if (constrain_match(cmp, lookup_port, aux, &m->match)) { > > expr_match_add(matches, m); > > } else { > > - free(m); > > + expr_match_destroy(m); > > } > > } > > > > @@ -3214,8 +3264,7 @@ expr_matches_destroy(struct hmap *matches) > > } > > > > HMAP_FOR_EACH_POP (m, hmap_node, matches) { > > - free(m->conjunctions); > > - free(m); > > + expr_match_destroy(m); > > } > > hmap_destroy(matches); > > } > > -- > > 2.30.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/lflow.c b/controller/lflow.c index 8b32c7469..79652735c 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -692,13 +692,23 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, } } } + + struct addrset_info as_info = { + .name = m->as_name, + .ip = m->as_ip, + .mask = m->as_mask + }; if (!m->n) { ofctrl_add_flow_metered(l_ctx_out->flow_table, ptable, lflow->priority, lflow->header_.uuid.parts[0], &m->match, &ofpacts, &lflow->header_.uuid, - ctrl_meter_id); + ctrl_meter_id, + as_info.name ? &as_info : NULL); } else { + if (m->n > 1) { + ovs_assert(!as_info.name); + } uint64_t conj_stubs[64 / 8]; struct ofpbuf conj; @@ -716,7 +726,8 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable, lflow->priority, 0, &m->match, &conj, &lflow->header_.uuid, - ctrl_meter_id); + ctrl_meter_id, + as_info.name ? &as_info : NULL); ofpbuf_uninit(&conj); } } @@ -1524,7 +1535,7 @@ add_lb_ct_snat_hairpin_dp_flows(struct ovn_controller_lb *lb, ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, 200, lb->slb->header_.uuid.parts[0], &dp_match, &dp_acts, &lb->slb->header_.uuid, - NX_CTLR_NO_METER); + NX_CTLR_NO_METER, NULL); } ofpbuf_uninit(&dp_acts); @@ -1698,7 +1709,7 @@ add_lb_ct_snat_hairpin_vip_flow(struct ovn_controller_lb *lb, ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN, priority, lb->slb->header_.uuid.parts[0], &match, &ofpacts, &lb->slb->header_.uuid, - NX_CTLR_NO_METER); + NX_CTLR_NO_METER, NULL); ofpbuf_uninit(&ofpacts); } diff --git a/controller/ofctrl.c b/controller/ofctrl.c index bcd6cea79..7671a3b7a 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -165,15 +165,35 @@ struct sb_to_flow { struct uuid sb_uuid; struct ovs_list flows; /* A list of struct sb_flow_ref nodes that are referenced by the sb_uuid. */ + struct ovs_list addrsets; /* A list of struct sb_addrset_ref. */ }; struct sb_flow_ref { struct ovs_list sb_list; /* List node in desired_flow.references. */ - struct ovs_list flow_list; /* List node in sb_to_flow.desired_flows. */ + struct ovs_list flow_list; /* List node in sb_to_flow.flows. */ + struct ovs_list as_ip_flow_list; /* List node in ip_to_flow_node.flows. */ struct desired_flow *flow; struct uuid sb_uuid; }; +struct sb_addrset_ref { + struct ovs_list list_node; /* List node in sb_to_flow.addrsets. */ + char *name; /* Name of the address set. */ + struct hmap ip_to_flow_map; /* map from IPs in the address set to flows. + Each node is struct ip_to_flow_node. */ +}; + +struct ip_to_flow_node { + struct hmap_node hmap_node; /* Node in sb_addrset_ref.ip_to_flow_map. */ + struct in6_addr as_ip; + struct in6_addr as_mask; + + /* A list of struct sb_flow_ref. A single IP in an address set can be + * used by multiple flows. e.g., in match: + * ip.src == $as1 && ip.dst == $as1. */ + struct ovs_list flows; +}; + /* An installed flow, in static variable installed_lflows/installed_pflows. * * Installed flows are updated in ofctrl_put for maintaining the flow @@ -1030,9 +1050,26 @@ sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid *sb_uuid) return NULL; } +static struct ip_to_flow_node * +ip_to_flow_find(struct hmap *ip_to_flow_map, const struct in6_addr *as_ip, + const struct in6_addr *as_mask) +{ + uint32_t hash = hash_bytes(as_ip, sizeof *as_ip, 0); + + struct ip_to_flow_node *itfn; + HMAP_FOR_EACH_WITH_HASH (itfn, hmap_node, hash, ip_to_flow_map) { + if (ipv6_addr_equals(&itfn->as_ip, as_ip) + && ipv6_addr_equals(&itfn->as_mask, as_mask)) { + return itfn; + } + } + return NULL; +} + static void link_flow_to_sb(struct ovn_desired_flow_table *flow_table, - struct desired_flow *f, const struct uuid *sb_uuid) + struct desired_flow *f, const struct uuid *sb_uuid, + const struct addrset_info *as_info) { struct sb_flow_ref *sfr = xmalloc(sizeof *sfr); mem_stats.sb_flow_ref_usage += sb_flow_ref_size(sfr); @@ -1046,10 +1083,48 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, mem_stats.sb_flow_ref_usage += sb_to_flow_size(stf); stf->sb_uuid = *sb_uuid; ovs_list_init(&stf->flows); + ovs_list_init(&stf->addrsets); hmap_insert(&flow_table->uuid_flow_table, &stf->hmap_node, uuid_hash(sb_uuid)); } ovs_list_insert(&stf->flows, &sfr->flow_list); + + if (!as_info) { + ovs_list_init(&sfr->as_ip_flow_list); + return; + } + + /* link flow to address_set + ip */ + struct sb_addrset_ref *sar; + bool found = false; + LIST_FOR_EACH (sar, list_node, &stf->addrsets) { + if (!strcmp(sar->name, as_info->name)) { + found = true; + break; + } + } + if (!found) { + sar = xmalloc(sizeof *sar); + mem_stats.sb_flow_ref_usage += sizeof *sar; + sar->name = xstrdup(as_info->name); + hmap_init(&sar->ip_to_flow_map); + ovs_list_insert(&stf->addrsets, &sar->list_node); + } + + struct ip_to_flow_node * itfn = ip_to_flow_find(&sar->ip_to_flow_map, + &as_info->ip, + &as_info->mask); + if (!itfn) { + itfn = xmalloc(sizeof *itfn); + mem_stats.sb_flow_ref_usage += sizeof *itfn; + itfn->as_ip = as_info->ip; + itfn->as_mask = as_info->mask; + ovs_list_init(&itfn->flows); + uint32_t hash = hash_bytes(&as_info->ip, sizeof as_info->ip, 0); + hmap_insert(&sar->ip_to_flow_map, &itfn->hmap_node, hash); + } + + ovs_list_insert(&itfn->flows, &sfr->as_ip_flow_list); } /* Flow table interfaces to the rest of ovn-controller. */ @@ -1068,13 +1143,17 @@ link_flow_to_sb(struct ovn_desired_flow_table *flow_table, void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, uint8_t table_id, uint16_t priority, - uint64_t cookie, const struct match *match, + uint64_t cookie, + const struct match *match, const struct ofpbuf *actions, const struct uuid *sb_uuid, - uint32_t meter_id, bool log_duplicate_flow) + uint32_t meter_id, + const struct addrset_info *as_info, + bool log_duplicate_flow) { struct desired_flow *f = desired_flow_alloc(table_id, priority, cookie, - match, actions, meter_id); + match, actions, + meter_id); if (desired_flow_lookup_check_uuid(flow_table, &f->flow, sb_uuid)) { if (log_duplicate_flow) { @@ -1091,7 +1170,7 @@ ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *flow_table, hmap_insert(&flow_table->match_flow_table, &f->match_hmap_node, f->flow.hash); - link_flow_to_sb(flow_table, f, sb_uuid); + link_flow_to_sb(flow_table, f, sb_uuid, as_info); track_flow_add_or_modify(flow_table, f); ovn_flow_log(&f->flow, "ofctrl_add_flow"); } @@ -1103,7 +1182,7 @@ ofctrl_add_flow(struct ovn_desired_flow_table *desired_flows, const struct uuid *sb_uuid) { ofctrl_add_flow_metered(desired_flows, table_id, priority, cookie, - match, actions, sb_uuid, NX_CTLR_NO_METER); + match, actions, sb_uuid, NX_CTLR_NO_METER, NULL); } void @@ -1111,11 +1190,12 @@ ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, uint8_t table_id, uint16_t priority, uint64_t cookie, const struct match *match, const struct ofpbuf *actions, - const struct uuid *sb_uuid, uint32_t meter_id) + const struct uuid *sb_uuid, uint32_t meter_id, + const struct addrset_info *as_info) { ofctrl_check_and_add_flow_metered(desired_flows, table_id, priority, cookie, match, actions, sb_uuid, - meter_id, true); + meter_id, as_info, true); } /* Either add a new flow, or append actions on an existing flow. If the @@ -1127,7 +1207,8 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, const struct match *match, const struct ofpbuf *actions, const struct uuid *sb_uuid, - uint32_t meter_id) + uint32_t meter_id, + const struct addrset_info *as_info) { struct desired_flow *existing; struct desired_flow *f; @@ -1156,11 +1237,20 @@ ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, ofpbuf_uninit(&compound); desired_flow_destroy(f); f = existing; + + /* Remove as_info tracking for the existing flow. */ + struct sb_flow_ref *sfr; + LIST_FOR_EACH (sfr, sb_list, &f->references) { + ovs_list_remove(&sfr->as_ip_flow_list); + ovs_list_init(&sfr->as_ip_flow_list); + } + /* Link to sb but don't track the as_info. */ + link_flow_to_sb(desired_flows, f, sb_uuid, NULL); } else { hmap_insert(&desired_flows->match_flow_table, &f->match_hmap_node, f->flow.hash); + link_flow_to_sb(desired_flows, f, sb_uuid, as_info); } - link_flow_to_sb(desired_flows, f, sb_uuid); track_flow_add_or_modify(desired_flows, f); if (existing) { @@ -1269,6 +1359,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, LIST_FOR_EACH_SAFE (sfr, next, flow_list, &stf->flows) { ovs_list_remove(&sfr->sb_list); ovs_list_remove(&sfr->flow_list); + ovs_list_remove(&sfr->as_ip_flow_list); struct desired_flow *f = sfr->flow; mem_stats.sb_flow_ref_usage -= sb_flow_ref_size(sfr); free(sfr); @@ -1286,6 +1377,22 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, } } + struct sb_addrset_ref *sar, *next_sar; + LIST_FOR_EACH_SAFE (sar, next_sar, list_node, &stf->addrsets) { + ovs_list_remove(&sar->list_node); + struct ip_to_flow_node *itfn, *itfn_next; + HMAP_FOR_EACH_SAFE (itfn, itfn_next, hmap_node, &sar->ip_to_flow_map) { + hmap_remove(&sar->ip_to_flow_map, &itfn->hmap_node); + ovs_assert(ovs_list_is_empty(&itfn->flows)); + mem_stats.sb_flow_ref_usage -= sizeof *itfn; + free(itfn); + } + hmap_destroy(&sar->ip_to_flow_map); + mem_stats.sb_flow_ref_usage -= (sizeof *sar + strlen(sar->name) + 1); + free(sar->name); + free(sar); + } + hmap_remove(&flow_table->uuid_flow_table, &stf->hmap_node); mem_stats.sb_flow_ref_usage -= sb_to_flow_size(stf); free(stf); @@ -1300,6 +1407,7 @@ remove_flows_from_sb_to_flow(struct ovn_desired_flow_table *flow_table, ovs_assert(!ovs_list_is_empty(&f->references)); LIST_FOR_EACH (sfr, sb_list, &f->references) { ovs_list_remove(&sfr->flow_list); + ovs_list_remove(&sfr->as_ip_flow_list); } } LIST_FOR_EACH_SAFE (f, f_next, list_node, &to_be_removed) { diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 014de210d..4ec328c24 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -71,24 +71,34 @@ char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const struct shash *port_groups); /* Flow table interfaces to the rest of ovn-controller. */ + +/* Information of IP of an address set used to track a flow that is generated + * from a logical flow referencing address set(s). */ +struct addrset_info { + const char *name; /* The address set's name. */ + struct in6_addr ip; /* An IP in the address set. */ + struct in6_addr mask; /* The mask of the IP. */ +}; void ofctrl_add_flow(struct ovn_desired_flow_table *, uint8_t table_id, uint16_t priority, uint64_t cookie, const struct match *, const struct ofpbuf *ofpacts, const struct uuid *); -void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows, +void ofctrl_add_or_append_flow(struct ovn_desired_flow_table *, uint8_t table_id, uint16_t priority, - uint64_t cookie, const struct match *match, + uint64_t cookie, const struct match *, const struct ofpbuf *actions, const struct uuid *sb_uuid, - uint32_t meter_id); + uint32_t meter_id, + const struct addrset_info *); void ofctrl_add_flow_metered(struct ovn_desired_flow_table *desired_flows, uint8_t table_id, uint16_t priority, uint64_t cookie, const struct match *match, const struct ofpbuf *actions, const struct uuid *sb_uuid, - uint32_t meter_id); + uint32_t meter_id, + const struct addrset_info *); /* Removes a bundles of flows from the flow table for a specific sb_uuid. The * flows are removed only if they are not referenced by any other sb_uuid(s). @@ -123,6 +133,7 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, uint64_t cookie, const struct match *, const struct ofpbuf *ofpacts, const struct uuid *, uint32_t meter_id, + const struct addrset_info *, bool log_duplicate_flow); diff --git a/controller/physical.c b/controller/physical.c index 6bfa2304d..033828d57 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -835,7 +835,7 @@ put_local_common_flows(uint32_t dp_key, put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p)); ofctrl_check_and_add_flow_metered(flow_table, OFTABLE_SAVE_INPORT, 100, 0, &match, ofpacts_p, hc_uuid, - NX_CTLR_NO_METER, false); + NX_CTLR_NO_METER, NULL, false); } } diff --git a/include/ovn/expr.h b/include/ovn/expr.h index 3b5653f7b..5572a1071 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -367,6 +367,8 @@ bool expr_relop_from_token(enum lex_type type, enum expr_relop *relop); struct expr { struct ovs_list node; /* In parent EXPR_T_AND or EXPR_T_OR if any. */ enum expr_type type; /* Expression type. */ + char *as_name; /* Address set name. Null if it is not an + address set. */ union { /* EXPR_T_CMP. @@ -469,6 +471,11 @@ struct expr_match { struct match match; struct cls_conjunction *conjunctions; size_t n, allocated; + + /* Tracked address set information. */ + char *as_name; + struct in6_addr as_ip; + struct in6_addr as_mask; }; uint32_t expr_to_matches(const struct expr *, @@ -526,6 +533,8 @@ struct expr_constant_set { size_t n_values; /* Number of constants. */ enum expr_constant_type type; /* Type of the constants. */ bool in_curlies; /* Whether the constants were in {}. */ + char *as_name; /* Name of an address set. It is NULL if not + an address set. */ }; bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *); diff --git a/lib/expr.c b/lib/expr.c index 5fc6c1ce9..af083190f 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -160,7 +160,7 @@ expr_relop_test(enum expr_relop relop, int cmp) struct expr * expr_create_andor(enum expr_type type) { - struct expr *e = xmalloc(sizeof *e); + struct expr *e = xzalloc(sizeof *e); e->type = type; ovs_list_init(&e->andor); return e; @@ -190,9 +190,17 @@ expr_combine(enum expr_type type, struct expr *a, struct expr *b) } else { ovs_list_push_back(&a->andor, &b->node); } + if (a->as_name) { + free(a->as_name); + a->as_name = NULL; + } return a; } else if (b->type == type) { ovs_list_push_front(&b->andor, &a->node); + if (b->as_name) { + free(b->as_name); + b->as_name = NULL; + } return b; } else { struct expr *e = expr_create_andor(type); @@ -220,7 +228,7 @@ expr_insert_andor(struct expr *andor, struct expr *before, struct expr *new) struct expr * expr_create_boolean(bool b) { - struct expr *e = xmalloc(sizeof *e); + struct expr *e = xzalloc(sizeof *e); e->type = EXPR_T_BOOLEAN; e->boolean = b; return e; @@ -680,6 +688,10 @@ make_cmp(struct expr_context *ctx, e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, e, make_cmp__(f, r, &cs->values[i])); } + /* Track address set */ + if (r == EXPR_R_EQ && e->type == EXPR_T_OR && cs->as_name) { + e->as_name = xstrdup(cs->as_name); + } exit: expr_constant_set_destroy(cs); return e; @@ -802,6 +814,10 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs, return false; } + if (!cs->n_values) { + cs->as_name = xstrdup(ctx->lexer->token.s); + } + size_t n_values = cs->n_values + addr_sets->n_values; if (n_values >= *allocated_values) { cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); @@ -875,6 +891,13 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, sizeof *cs->values); } + if (cs->as_name) { + /* Combining other values to the constant set that is tracking an + * address set, so untrack it. */ + free(cs->as_name); + cs->as_name = NULL; + } + if (ctx->lexer->token.type == LEX_T_STRING) { if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { return false; @@ -1057,6 +1080,7 @@ expr_constant_set_destroy(struct expr_constant_set *cs) } } free(cs->values); + free(cs->as_name); } } @@ -1244,6 +1268,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic) c.values = cst; c.n_values = 1; c.in_curlies = false; + c.as_name = NULL; return make_cmp(ctx, &f, EXPR_R_NE, &c); } else if (parse_relop(ctx, &r) && parse_constant_set(ctx, &c)) { return make_cmp(ctx, &f, r, &c); @@ -1709,6 +1734,7 @@ expr_symtab_destroy(struct shash *symtab) static struct expr * expr_clone_cmp(struct expr *expr) { + ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); if (!new->cmp.symbol->width) { new->cmp.string = xstrdup(new->cmp.string); @@ -1732,6 +1758,7 @@ expr_clone_andor(struct expr *expr) static struct expr * expr_clone_condition(struct expr *expr) { + ovs_assert(!expr->as_name); struct expr *new = xmemdup(expr, sizeof *expr); new->cond.string = xstrdup(new->cond.string); return new; @@ -1767,6 +1794,8 @@ expr_destroy(struct expr *expr) return; } + free(expr->as_name); + struct expr *sub, *next; switch (expr->type) { @@ -2373,7 +2402,7 @@ crush_and_string(struct expr *expr, const struct expr_symbol *symbol) const char *string; SSET_FOR_EACH (string, &result) { - sub = xmalloc(sizeof *sub); + sub = xzalloc(sizeof *sub); sub->type = EXPR_T_CMP; sub->cmp.relop = EXPR_R_EQ; sub->cmp.symbol = symbol; @@ -2432,7 +2461,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) return expr_create_boolean(true); } else { struct expr *cmp; - cmp = xmalloc(sizeof *cmp); + cmp = xzalloc(sizeof *cmp); cmp->type = EXPR_T_CMP; cmp->cmp.symbol = symbol; cmp->cmp.relop = EXPR_R_EQ; @@ -2447,7 +2476,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) struct expr *disjuncts = expr_from_node(ovs_list_pop_front(&expr->andor)); struct expr *or; - or = xmalloc(sizeof *or); + or = xzalloc(sizeof *or); or->type = EXPR_T_OR; ovs_list_init(&or->andor); @@ -2483,7 +2512,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) struct expr *new = NULL; struct expr *or; - or = xmalloc(sizeof *or); + or = xzalloc(sizeof *or); or->type = EXPR_T_OR; ovs_list_init(&or->andor); @@ -2502,7 +2531,7 @@ crush_and_numeric(struct expr *expr, const struct expr_symbol *symbol) LIST_FOR_EACH (b, node, &bs->andor) { ovs_assert(b->type == EXPR_T_CMP); if (!new) { - new = xmalloc(sizeof *new); + new = xzalloc(sizeof *new); new->type = EXPR_T_CMP; new->cmp.symbol = symbol; new->cmp.relop = EXPR_R_EQ; @@ -2608,6 +2637,9 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol) ovs_list_push_back(&expr->andor, &b->node); } else { expr_destroy(b); + /* Member modified, so untrack address set. */ + free(expr->as_name); + expr->as_name = NULL; } } free(subs); @@ -2659,7 +2691,7 @@ expr_sort(struct expr *expr) ovs_assert(expr->type == EXPR_T_AND); size_t n = ovs_list_size(&expr->andor); - struct expr_sort *subs = xmalloc(n * sizeof *subs); + struct expr_sort *subs = xzalloc(n * sizeof *subs); struct expr *sub; size_t i; @@ -2884,7 +2916,7 @@ static struct expr_match * expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, uint32_t conj_id) { - struct expr_match *match = xmalloc(sizeof *match); + struct expr_match *match = xzalloc(sizeof *match); if (m) { match->match = *m; } else { @@ -2905,6 +2937,14 @@ expr_match_new(const struct match *m, uint8_t clause, uint8_t n_clauses, return match; } +static void +expr_match_destroy(struct expr_match *match) +{ + free(match->as_name); + free(match->conjunctions); + free(match); +} + /* Adds 'match' to hash table 'matches', which becomes the new owner of * 'match'. * @@ -2932,8 +2972,12 @@ expr_match_add(struct hmap *matches, struct expr_match *match) } m->conjunctions[m->n++] = match->conjunctions[0]; } - free(match->conjunctions); - free(match); + if (m->as_name) { + /* m is combined with match. so untracked the address set. */ + free(m->as_name); + m->as_name = NULL; + } + expr_match_destroy(match); return; } } @@ -2994,12 +3038,18 @@ add_disjunction(const struct expr *or, LIST_FOR_EACH (sub, node, &or->andor) { struct expr_match *match = expr_match_new(m, clause, n_clauses, conj_id); + if (or->as_name) { + ovs_assert(sub->type == EXPR_T_CMP); + ovs_assert(sub->cmp.symbol->width); + match->as_name = xstrdup(or->as_name); + match->as_ip = sub->cmp.value.ipv6; + match->as_mask = sub->cmp.mask.ipv6; + } if (constrain_match(sub, lookup_port, aux, &match->match)) { expr_match_add(matches, match); n++; } else { - free(match->conjunctions); - free(match); + expr_match_destroy(match); } } @@ -3082,7 +3132,7 @@ add_cmp_flow(const struct expr *cmp, if (constrain_match(cmp, lookup_port, aux, &m->match)) { expr_match_add(matches, m); } else { - free(m); + expr_match_destroy(m); } } @@ -3214,8 +3264,7 @@ expr_matches_destroy(struct hmap *matches) } HMAP_FOR_EACH_POP (m, hmap_node, matches) { - free(m->conjunctions); - free(m); + expr_match_destroy(m); } hmap_destroy(matches); }
This patch tracks individual address information when parsing address sets from logical flows, and link to the corresponding desired flow resulted from the IP. Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/lflow.c | 19 ++++-- controller/ofctrl.c | 130 ++++++++++++++++++++++++++++++++++++++---- controller/ofctrl.h | 19 ++++-- controller/physical.c | 2 +- include/ovn/expr.h | 9 +++ lib/expr.c | 81 ++++++++++++++++++++------ 6 files changed, 224 insertions(+), 36 deletions(-)