On 10/13/20 9:03 AM, Han Zhou wrote: > > > On Sun, Oct 11, 2020 at 5:06 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: >> >> Always consider the first "desired flow" in the list of flows that > refer an >> "installed flow" as the active flow. This simplifies the logic of the > flow >> update code and is used in a further commit to implement a partial > ordering >> of desired flows within installed flows. >> >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> >> --- >> controller/ofctrl.c | 92 > +++++++++++++++++++++------------------------------ >> 1 file changed, 37 insertions(+), 55 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 20cf3ac..74f98e3 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -188,6 +188,8 @@ struct sb_flow_ref { >> * relationship is 1 to N. A link is added when a flow addition is > processed. >> * A link is removed when a flow deletion is processed, the desired flow >> * table is cleared, or the installed flow table is cleared. >> + * The first desired_flow in the list is the active one, the one that is >> + * actually installed. >> */ >> struct installed_flow { >> struct ovn_flow flow; >> @@ -199,11 +201,6 @@ struct installed_flow { >> * installed flow, e.g. when there are conflict/duplicated ACLs that >> * generates same match conditions). */ >> struct ovs_list desired_refs; >> - >> - /* The corresponding flow in desired table. It must be one of the > flows in >> - * desired_refs list. If there are more than one flows in > references list, >> - * this is the one that is actually installed. */ >> - struct desired_flow *desired_flow; >> }; >> >> typedef bool >> @@ -231,6 +228,8 @@ static struct installed_flow *installed_flow_lookup( >> const struct ovn_flow *target); >> static void installed_flow_destroy(struct installed_flow *); >> static struct installed_flow *installed_flow_dup(struct desired_flow *); >> +static struct desired_flow *installed_flow_get_active( >> + struct installed_flow *f); >> >> static uint32_t ovn_flow_match_hash(const struct ovn_flow *); >> static char *ovn_flow_to_string(const struct ovn_flow *); >> @@ -796,24 +795,6 @@ ofctrl_recv(const struct ofp_header *oh, enum > ofptype type) >> log_openflow_rl(&rl, VLL_DBG, oh, "OpenFlow packet ignored"); >> } >> } >> - >> -/* Returns true if a desired flow is active (the one currently installed >> - * among the list of desired flows that are linked to the same installed >> - * flow). */ >> -static inline bool >> -desired_flow_is_active(struct desired_flow *d) >> -{ >> - return (d->installed_flow && d->installed_flow->desired_flow == d); >> -} >> - >> -/* Set a desired flow as the active one among the list of desired flows >> - * that are linked to the same installed flow. */ >> -static inline void >> -desired_flow_set_active(struct desired_flow *d) >> -{ >> - ovs_assert(d->installed_flow); >> - d->installed_flow->desired_flow = d; >> -} >> >> static bool >> flow_action_has_conj(const struct ovn_flow *f) >> @@ -831,27 +812,22 @@ flow_action_has_conj(const struct ovn_flow *f) >> /* Adds the desired flow to the list of desired flows that have same > match >> * conditions as the installed flow. >> * >> - * If the newly added desired flow is the first one in the list, it > is also set >> - * as the active one. >> - * >> * It is caller's responsibility to make sure the link between the > pair didn't >> - * exist before. */ >> -static void >> + * exist before. >> + * >> + * Returns true if the newly added desired flow is selected to be the > active >> + * one. >> + */ >> +static bool >> link_installed_to_desired(struct installed_flow *i, struct > desired_flow *d) >> { >> - ovs_assert(i->desired_flow != d); >> - if (ovs_list_is_empty(&i->desired_refs)) { >> - ovs_assert(!i->desired_flow); >> - i->desired_flow = d; >> - } >> - ovs_list_insert(&i->desired_refs, &d->installed_ref_list_node); >> d->installed_flow = i; >> + ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node); >> + return installed_flow_get_active(i) == d; >> } >> >> /* Replaces 'old_desired' with 'new_desired' in the list of desired flows >> * that have same match conditions as the installed flow. >> - * >> - * If 'old_desired' was the active flow, 'new_desired' becomes the > active one. >> */ >> static void >> replace_installed_to_desired(struct installed_flow *i, >> @@ -863,24 +839,22 @@ replace_installed_to_desired(struct > installed_flow *i, >> &old_desired->installed_ref_list_node); >> old_desired->installed_flow = NULL; >> new_desired->installed_flow = i; >> - if (i->desired_flow == old_desired) { >> - i->desired_flow = new_desired; >> - } >> } >> >> -static void >> +/* Removes the desired flow from the list of desired flows that have > the same >> + * match conditions as the installed flow. >> + * >> + * Returns true if the desired flow was the previously active flow. >> + */ >> +static bool >> unlink_installed_to_desired(struct installed_flow *i, struct > desired_flow *d) >> { >> - ovs_assert(i && i->desired_flow && > !ovs_list_is_empty(&i->desired_refs)); >> + struct desired_flow *old_active = installed_flow_get_active(i); >> + >> ovs_assert(d && d->installed_flow == i); >> ovs_list_remove(&d->installed_ref_list_node); >> d->installed_flow = NULL; >> - if (i->desired_flow == d) { >> - i->desired_flow = ovs_list_is_empty(&i->desired_refs) ? NULL : >> - CONTAINER_OF(ovs_list_front(&i->desired_refs), >> - struct desired_flow, >> - installed_ref_list_node); >> - } >> + return old_active == d; >> } >> >> static void >> @@ -1280,7 +1254,6 @@ installed_flow_dup(struct desired_flow *src) >> { >> struct installed_flow *dst = xmalloc(sizeof *dst); >> ovs_list_init(&dst->desired_refs); >> - dst->desired_flow = NULL; >> dst->flow.table_id = src->flow.table_id; >> dst->flow.priority = src->flow.priority; >> minimatch_clone(&dst->flow.match, &src->flow.match); >> @@ -1292,6 +1265,17 @@ installed_flow_dup(struct desired_flow *src) >> } >> >> static struct desired_flow * >> +installed_flow_get_active(struct installed_flow *f) >> +{ >> + if (!ovs_list_is_empty(&f->desired_refs)) { >> + return CONTAINER_OF(ovs_list_front(&f->desired_refs), >> + struct desired_flow, >> + installed_ref_list_node); >> + } >> + return NULL; >> +} >> + >> +static struct desired_flow * >> desired_flow_lookup__(struct ovn_desired_flow_table *flow_table, >> const struct ovn_flow *target, >> desired_flow_match_cb match_cb, >> @@ -1439,8 +1423,7 @@ static void >> installed_flow_destroy(struct installed_flow *f) >> { >> if (f) { >> - ovs_assert(ovs_list_is_empty(&f->desired_refs)); >> - ovs_assert(!f->desired_flow); >> + ovs_assert(!installed_flow_get_active(f)); >> ovn_flow_uninit(&f->flow); >> free(f); >> } >> @@ -1898,10 +1881,10 @@ update_installed_flows_by_track(struct > ovn_desired_flow_table *flow_table, >> /* The desired flow was deleted */ >> if (f->installed_flow) { >> struct installed_flow *i = f->installed_flow; >> - bool was_active = desired_flow_is_active(f); >> - unlink_installed_to_desired(i, f); >> + bool was_active = unlink_installed_to_desired(i, f); >> + struct desired_flow *d = installed_flow_get_active(i); >> >> - if (!i->desired_flow) { >> + if (!d) { >> installed_flow_del(&i->flow, msgs); >> ovn_flow_log(&i->flow, "removing installed > (tracked)"); >> >> @@ -1912,7 +1895,6 @@ update_installed_flows_by_track(struct > ovn_desired_flow_table *flow_table, >> * installed flow, so update the OVS flow for the new >> * active flow (at least the cookie will be > different, >> * even if the actions are the same). */ >> - struct desired_flow *d = i->desired_flow; >> ovn_flow_log(&i->flow, "updating installed > (tracked)"); >> installed_flow_mod(&i->flow, &d->flow, msgs); >> } >> @@ -1931,7 +1913,7 @@ update_installed_flows_by_track(struct > ovn_desired_flow_table *flow_table, >> hmap_insert(&installed_flows, &new_node->match_hmap_node, >> new_node->flow.hash); >> link_installed_to_desired(new_node, f); >> - } else if (desired_flow_is_active(f)) { >> + } else if (installed_flow_get_active(i) == f) { >> /* The installed flow is installed for f, but f has > change >> * tracked, so it must have been modified. */ >> ovn_flow_log(&i->flow, "updating installed (tracked)"); >> > > Thanks Dumitru. I applied patch 1 - 3 of the series to master, with a > tiny change to patch 3 just to follow the coding style: > > ............................... 8>< > ................................................................><8 > .................................... > index 74f98e36b..ba0c61c80 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -228,8 +228,7 @@ static struct installed_flow *installed_flow_lookup( > const struct ovn_flow *target); > static void installed_flow_destroy(struct installed_flow *); > static struct installed_flow *installed_flow_dup(struct desired_flow *); > -static struct desired_flow *installed_flow_get_active( > - struct installed_flow *f); > +static struct desired_flow *installed_flow_get_active(struct > installed_flow *); > > static uint32_t ovn_flow_match_hash(const struct ovn_flow *); > static char *ovn_flow_to_string(const struct ovn_flow *);
Ack, looks good to me. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
