| Message ID | 20221202173147.3032702-2-odivlad@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN IC bugfixes & proposals/questions | 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 | success | github build: passed |
On 12/2/22 18:31, Vladislav Odintsov wrote: > This change will be useful in next commit. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- Hi Vladislav, This looks OK to me but I think I'd squash it in the patch that actually uses the new way of calling ic_route_find(). Thanks, Dumitru > ic/ovn-ic.c | 45 +++++++++++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index e5c193d9d..50ff65a26 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -881,10 +881,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int plen, > static struct ic_route_info * > ic_route_find(struct hmap *routes, const struct in6_addr *prefix, > unsigned int plen, const struct in6_addr *nexthop, > - const char *origin, char *route_table) > + const char *origin, const char *route_table, uint32_t hash) > { > struct ic_route_info *r; > - uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); > + if (!hash) { > + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); > + } > HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { > if (ipv6_addr_equals(&r->prefix, prefix) && > r->plen == plen && > @@ -942,8 +944,8 @@ add_to_routes_learned(struct hmap *routes_learned, > } > const char *origin = smap_get_def(&nb_route->options, "origin", ""); > if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, > - nb_route->route_table)) { > - /* Route is already added to learned in previous iteration. */ > + nb_route->route_table, 0)) { > + /* Route was added to learned on previous iteration. */ > return true; > } > > @@ -1090,10 +1092,21 @@ route_need_advertise(const char *policy, > } > > static void > -add_to_routes_ad(struct hmap *routes_ad, > - const struct nbrec_logical_router_static_route *nb_route, > - const struct lport_addresses *nexthop_addresses, > - const struct smap *nb_options, const char *route_table) > +add_to_routes_ad(struct hmap *routes_ad, struct ic_route_info *ic_route) > +{ > + uint hash = ic_route_hash(&ic_route->prefix, ic_route->plen, > + &ic_route->nexthop, ic_route->origin, > + ic_route->route_table ? ic_route->route_table > + : ""); > + hmap_insert(routes_ad, &ic_route->node, hash); > +} > + > +static void > +add_static_to_routes_ad( > + struct hmap *routes_ad, > + const struct nbrec_logical_router_static_route *nb_route, > + const struct lport_addresses *nexthop_addresses, > + const struct smap *nb_options, const char *route_table) > { > if (strcmp(route_table, nb_route->route_table)) { > if (VLOG_IS_DBG_ENABLED()) { > @@ -1149,9 +1162,7 @@ add_to_routes_ad(struct hmap *routes_ad, > ic_route->nb_route = nb_route; > ic_route->origin = ROUTE_ORIGIN_STATIC; > ic_route->route_table = nb_route->route_table; > - hmap_insert(routes_ad, &ic_route->node, > - ic_route_hash(&prefix, plen, &nexthop, ROUTE_ORIGIN_STATIC, > - nb_route->route_table)); > + add_to_routes_ad(routes_ad, ic_route); > } > > static void > @@ -1204,9 +1215,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const char *network, > > /* directly-connected routes go to <main> route table */ > ic_route->route_table = NULL; > - hmap_insert(routes_ad, &ic_route->node, > - ic_route_hash(&prefix, plen, &nexthop, > - ROUTE_ORIGIN_CONNECTED, "")); > + add_to_routes_ad(routes_ad, ic_route); > } > > static bool > @@ -1366,7 +1375,7 @@ sync_learned_routes(struct ic_context *ctx, > struct ic_route_info *route_learned > = ic_route_find(&ic_lr->routes_learned, &prefix, plen, > &nexthop, isb_route->origin, > - isb_route->route_table); > + isb_route->route_table, 0); > if (route_learned) { > /* Sync external-ids */ > struct uuid ext_id; > @@ -1465,7 +1474,7 @@ advertise_routes(struct ic_context *ctx, > } > struct ic_route_info *route_adv = > ic_route_find(routes_ad, &prefix, plen, &nexthop, > - isb_route->origin, isb_route->route_table); > + isb_route->origin, isb_route->route_table, 0); > if (!route_adv) { > /* Delete the extra route from IC-SB. */ > VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" > @@ -1547,8 +1556,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, > } > } else { > /* It may be a route to be advertised */ > - add_to_routes_ad(routes_ad, nb_route, ts_port_addrs, > - &nb_global->options, ts_route_table); > + add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, > + &nb_global->options, ts_route_table); > } > } >
On Mon, Dec 5, 2022 at 11:37 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 12/2/22 18:31, Vladislav Odintsov wrote: > > This change will be useful in next commit. > > > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > --- > > Hi Vladislav, > > This looks OK to me but I think I'd squash it in the patch that actually > uses the new way of calling ic_route_find(). +1 for this. I'd also suggest splitting this series into 2. Patch 1, 2, 3, 5 and 7 into 1 series since these patches are fixing ic related issues. These can be backported easily to older branches. Patch 4 and 6 can be a separate patch series independent of these. I think these 2 patches need to be carefully reviewed. @Dumitru Ceara Do you have any objections ? Thanks for identifying these issues and fixing them. Thanks Numan > > Thanks, > Dumitru > > > ic/ovn-ic.c | 45 +++++++++++++++++++++++++++------------------ > > 1 file changed, 27 insertions(+), 18 deletions(-) > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > index e5c193d9d..50ff65a26 100644 > > --- a/ic/ovn-ic.c > > +++ b/ic/ovn-ic.c > > @@ -881,10 +881,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int plen, > > static struct ic_route_info * > > ic_route_find(struct hmap *routes, const struct in6_addr *prefix, > > unsigned int plen, const struct in6_addr *nexthop, > > - const char *origin, char *route_table) > > + const char *origin, const char *route_table, uint32_t hash) > > { > > struct ic_route_info *r; > > - uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); > > + if (!hash) { > > + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); > > + } > > HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { > > if (ipv6_addr_equals(&r->prefix, prefix) && > > r->plen == plen && > > @@ -942,8 +944,8 @@ add_to_routes_learned(struct hmap *routes_learned, > > } > > const char *origin = smap_get_def(&nb_route->options, "origin", ""); > > if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, > > - nb_route->route_table)) { > > - /* Route is already added to learned in previous iteration. */ > > + nb_route->route_table, 0)) { > > + /* Route was added to learned on previous iteration. */ > > return true; > > } > > > > @@ -1090,10 +1092,21 @@ route_need_advertise(const char *policy, > > } > > > > static void > > -add_to_routes_ad(struct hmap *routes_ad, > > - const struct nbrec_logical_router_static_route *nb_route, > > - const struct lport_addresses *nexthop_addresses, > > - const struct smap *nb_options, const char *route_table) > > +add_to_routes_ad(struct hmap *routes_ad, struct ic_route_info *ic_route) > > +{ > > + uint hash = ic_route_hash(&ic_route->prefix, ic_route->plen, > > + &ic_route->nexthop, ic_route->origin, > > + ic_route->route_table ? ic_route->route_table > > + : ""); > > + hmap_insert(routes_ad, &ic_route->node, hash); > > +} > > + > > +static void > > +add_static_to_routes_ad( > > + struct hmap *routes_ad, > > + const struct nbrec_logical_router_static_route *nb_route, > > + const struct lport_addresses *nexthop_addresses, > > + const struct smap *nb_options, const char *route_table) > > { > > if (strcmp(route_table, nb_route->route_table)) { > > if (VLOG_IS_DBG_ENABLED()) { > > @@ -1149,9 +1162,7 @@ add_to_routes_ad(struct hmap *routes_ad, > > ic_route->nb_route = nb_route; > > ic_route->origin = ROUTE_ORIGIN_STATIC; > > ic_route->route_table = nb_route->route_table; > > - hmap_insert(routes_ad, &ic_route->node, > > - ic_route_hash(&prefix, plen, &nexthop, ROUTE_ORIGIN_STATIC, > > - nb_route->route_table)); > > + add_to_routes_ad(routes_ad, ic_route); > > } > > > > static void > > @@ -1204,9 +1215,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const char *network, > > > > /* directly-connected routes go to <main> route table */ > > ic_route->route_table = NULL; > > - hmap_insert(routes_ad, &ic_route->node, > > - ic_route_hash(&prefix, plen, &nexthop, > > - ROUTE_ORIGIN_CONNECTED, "")); > > + add_to_routes_ad(routes_ad, ic_route); > > } > > > > static bool > > @@ -1366,7 +1375,7 @@ sync_learned_routes(struct ic_context *ctx, > > struct ic_route_info *route_learned > > = ic_route_find(&ic_lr->routes_learned, &prefix, plen, > > &nexthop, isb_route->origin, > > - isb_route->route_table); > > + isb_route->route_table, 0); > > if (route_learned) { > > /* Sync external-ids */ > > struct uuid ext_id; > > @@ -1465,7 +1474,7 @@ advertise_routes(struct ic_context *ctx, > > } > > struct ic_route_info *route_adv = > > ic_route_find(routes_ad, &prefix, plen, &nexthop, > > - isb_route->origin, isb_route->route_table); > > + isb_route->origin, isb_route->route_table, 0); > > if (!route_adv) { > > /* Delete the extra route from IC-SB. */ > > VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" > > @@ -1547,8 +1556,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, > > } > > } else { > > /* It may be a route to be advertised */ > > - add_to_routes_ad(routes_ad, nb_route, ts_port_addrs, > > - &nb_global->options, ts_route_table); > > + add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, > > + &nb_global->options, ts_route_table); > > } > > } > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi, Okay, I’ll split these patches in two series and squash patch #1 with patch #7. Regards, Vladislav Odintsov > On 5 Dec 2022, at 20:00, Numan Siddique <numans@ovn.org> wrote: > > On Mon, Dec 5, 2022 at 11:37 AM Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>> wrote: >> >> On 12/2/22 18:31, Vladislav Odintsov wrote: >>> This change will be useful in next commit. >>> >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> >>> --- >> >> Hi Vladislav, >> >> This looks OK to me but I think I'd squash it in the patch that actually >> uses the new way of calling ic_route_find(). > > +1 for this. > > I'd also suggest splitting this series into 2. > > Patch 1, 2, 3, 5 and 7 into 1 series since these patches are fixing ic > related issues. > These can be backported easily to older branches. > > Patch 4 and 6 can be a separate patch series independent of these. I > think these 2 patches > need to be carefully reviewed. > > @Dumitru Ceara Do you have any objections ? > > Thanks for identifying these issues and fixing them. > > Thanks > Numan > >> >> Thanks, >> Dumitru >> >>> ic/ovn-ic.c | 45 +++++++++++++++++++++++++++------------------ >>> 1 file changed, 27 insertions(+), 18 deletions(-) >>> >>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >>> index e5c193d9d..50ff65a26 100644 >>> --- a/ic/ovn-ic.c >>> +++ b/ic/ovn-ic.c >>> @@ -881,10 +881,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int plen, >>> static struct ic_route_info * >>> ic_route_find(struct hmap *routes, const struct in6_addr *prefix, >>> unsigned int plen, const struct in6_addr *nexthop, >>> - const char *origin, char *route_table) >>> + const char *origin, const char *route_table, uint32_t hash) >>> { >>> struct ic_route_info *r; >>> - uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); >>> + if (!hash) { >>> + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); >>> + } >>> HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { >>> if (ipv6_addr_equals(&r->prefix, prefix) && >>> r->plen == plen && >>> @@ -942,8 +944,8 @@ add_to_routes_learned(struct hmap *routes_learned, >>> } >>> const char *origin = smap_get_def(&nb_route->options, "origin", ""); >>> if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, >>> - nb_route->route_table)) { >>> - /* Route is already added to learned in previous iteration. */ >>> + nb_route->route_table, 0)) { >>> + /* Route was added to learned on previous iteration. */ >>> return true; >>> } >>> >>> @@ -1090,10 +1092,21 @@ route_need_advertise(const char *policy, >>> } >>> >>> static void >>> -add_to_routes_ad(struct hmap *routes_ad, >>> - const struct nbrec_logical_router_static_route *nb_route, >>> - const struct lport_addresses *nexthop_addresses, >>> - const struct smap *nb_options, const char *route_table) >>> +add_to_routes_ad(struct hmap *routes_ad, struct ic_route_info *ic_route) >>> +{ >>> + uint hash = ic_route_hash(&ic_route->prefix, ic_route->plen, >>> + &ic_route->nexthop, ic_route->origin, >>> + ic_route->route_table ? ic_route->route_table >>> + : ""); >>> + hmap_insert(routes_ad, &ic_route->node, hash); >>> +} >>> + >>> +static void >>> +add_static_to_routes_ad( >>> + struct hmap *routes_ad, >>> + const struct nbrec_logical_router_static_route *nb_route, >>> + const struct lport_addresses *nexthop_addresses, >>> + const struct smap *nb_options, const char *route_table) >>> { >>> if (strcmp(route_table, nb_route->route_table)) { >>> if (VLOG_IS_DBG_ENABLED()) { >>> @@ -1149,9 +1162,7 @@ add_to_routes_ad(struct hmap *routes_ad, >>> ic_route->nb_route = nb_route; >>> ic_route->origin = ROUTE_ORIGIN_STATIC; >>> ic_route->route_table = nb_route->route_table; >>> - hmap_insert(routes_ad, &ic_route->node, >>> - ic_route_hash(&prefix, plen, &nexthop, ROUTE_ORIGIN_STATIC, >>> - nb_route->route_table)); >>> + add_to_routes_ad(routes_ad, ic_route); >>> } >>> >>> static void >>> @@ -1204,9 +1215,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const char *network, >>> >>> /* directly-connected routes go to <main> route table */ >>> ic_route->route_table = NULL; >>> - hmap_insert(routes_ad, &ic_route->node, >>> - ic_route_hash(&prefix, plen, &nexthop, >>> - ROUTE_ORIGIN_CONNECTED, "")); >>> + add_to_routes_ad(routes_ad, ic_route); >>> } >>> >>> static bool >>> @@ -1366,7 +1375,7 @@ sync_learned_routes(struct ic_context *ctx, >>> struct ic_route_info *route_learned >>> = ic_route_find(&ic_lr->routes_learned, &prefix, plen, >>> &nexthop, isb_route->origin, >>> - isb_route->route_table); >>> + isb_route->route_table, 0); >>> if (route_learned) { >>> /* Sync external-ids */ >>> struct uuid ext_id; >>> @@ -1465,7 +1474,7 @@ advertise_routes(struct ic_context *ctx, >>> } >>> struct ic_route_info *route_adv = >>> ic_route_find(routes_ad, &prefix, plen, &nexthop, >>> - isb_route->origin, isb_route->route_table); >>> + isb_route->origin, isb_route->route_table, 0); >>> if (!route_adv) { >>> /* Delete the extra route from IC-SB. */ >>> VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" >>> @@ -1547,8 +1556,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, >>> } >>> } else { >>> /* It may be a route to be advertised */ >>> - add_to_routes_ad(routes_ad, nb_route, ts_port_addrs, >>> - &nb_global->options, ts_route_table); >>> + add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, >>> + &nb_global->options, ts_route_table); >>> } >>> } >>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On 12/5/22 18:00, Numan Siddique wrote: > On Mon, Dec 5, 2022 at 11:37 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 12/2/22 18:31, Vladislav Odintsov wrote: >>> This change will be useful in next commit. >>> >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >>> --- >> >> Hi Vladislav, >> >> This looks OK to me but I think I'd squash it in the patch that actually >> uses the new way of calling ic_route_find(). > > +1 for this. > > I'd also suggest splitting this series into 2. > > Patch 1, 2, 3, 5 and 7 into 1 series since these patches are fixing ic > related issues. > These can be backported easily to older branches. > > Patch 4 and 6 can be a separate patch series independent of these. I > think these 2 patches > need to be carefully reviewed. > > @Dumitru Ceara Do you have any objections ? No objections, sounds good to me. > > Thanks for identifying these issues and fixing them. > +1, thanks Vladislav and Anton! > Thanks > Numan >
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index e5c193d9d..50ff65a26 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -881,10 +881,12 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int plen, static struct ic_route_info * ic_route_find(struct hmap *routes, const struct in6_addr *prefix, unsigned int plen, const struct in6_addr *nexthop, - const char *origin, char *route_table) + const char *origin, const char *route_table, uint32_t hash) { struct ic_route_info *r; - uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); + if (!hash) { + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); + } HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { if (ipv6_addr_equals(&r->prefix, prefix) && r->plen == plen && @@ -942,8 +944,8 @@ add_to_routes_learned(struct hmap *routes_learned, } const char *origin = smap_get_def(&nb_route->options, "origin", ""); if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, - nb_route->route_table)) { - /* Route is already added to learned in previous iteration. */ + nb_route->route_table, 0)) { + /* Route was added to learned on previous iteration. */ return true; } @@ -1090,10 +1092,21 @@ route_need_advertise(const char *policy, } static void -add_to_routes_ad(struct hmap *routes_ad, - const struct nbrec_logical_router_static_route *nb_route, - const struct lport_addresses *nexthop_addresses, - const struct smap *nb_options, const char *route_table) +add_to_routes_ad(struct hmap *routes_ad, struct ic_route_info *ic_route) +{ + uint hash = ic_route_hash(&ic_route->prefix, ic_route->plen, + &ic_route->nexthop, ic_route->origin, + ic_route->route_table ? ic_route->route_table + : ""); + hmap_insert(routes_ad, &ic_route->node, hash); +} + +static void +add_static_to_routes_ad( + struct hmap *routes_ad, + const struct nbrec_logical_router_static_route *nb_route, + const struct lport_addresses *nexthop_addresses, + const struct smap *nb_options, const char *route_table) { if (strcmp(route_table, nb_route->route_table)) { if (VLOG_IS_DBG_ENABLED()) { @@ -1149,9 +1162,7 @@ add_to_routes_ad(struct hmap *routes_ad, ic_route->nb_route = nb_route; ic_route->origin = ROUTE_ORIGIN_STATIC; ic_route->route_table = nb_route->route_table; - hmap_insert(routes_ad, &ic_route->node, - ic_route_hash(&prefix, plen, &nexthop, ROUTE_ORIGIN_STATIC, - nb_route->route_table)); + add_to_routes_ad(routes_ad, ic_route); } static void @@ -1204,9 +1215,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const char *network, /* directly-connected routes go to <main> route table */ ic_route->route_table = NULL; - hmap_insert(routes_ad, &ic_route->node, - ic_route_hash(&prefix, plen, &nexthop, - ROUTE_ORIGIN_CONNECTED, "")); + add_to_routes_ad(routes_ad, ic_route); } static bool @@ -1366,7 +1375,7 @@ sync_learned_routes(struct ic_context *ctx, struct ic_route_info *route_learned = ic_route_find(&ic_lr->routes_learned, &prefix, plen, &nexthop, isb_route->origin, - isb_route->route_table); + isb_route->route_table, 0); if (route_learned) { /* Sync external-ids */ struct uuid ext_id; @@ -1465,7 +1474,7 @@ advertise_routes(struct ic_context *ctx, } struct ic_route_info *route_adv = ic_route_find(routes_ad, &prefix, plen, &nexthop, - isb_route->origin, isb_route->route_table); + isb_route->origin, isb_route->route_table, 0); if (!route_adv) { /* Delete the extra route from IC-SB. */ VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" @@ -1547,8 +1556,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, } } else { /* It may be a route to be advertised */ - add_to_routes_ad(routes_ad, nb_route, ts_port_addrs, - &nb_global->options, ts_route_table); + add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, + &nb_global->options, ts_route_table); } }
This change will be useful in next commit. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- ic/ovn-ic.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)