Hi Mark, thank you for the review.
On Mon, Jul 25, 2022 at 6:09 PM Mark Michelson <[email protected]> wrote: > Hi Ales, I have one small finding below, but other than that, it looks > good. I don't think you need to roll a new version to address this issue, > so > > Acked-by: Mark Michelson <[email protected]> > > On 7/25/22 06:49, Ales Musil wrote: > > The local_datapaths are populated by iterating through > > all available port bindings or new ones added during I-C. > > The port bindings are not ordered, it might happen that > > the order between engine runs are different. This can cause > > the prefix delegation shash to be with NULL local_datapath. > > Even in cases that the port binding is actually part of > > the local_datapath. Store only port binding in the shash > > and during pinctrl_run check if the stored port binding > > is part of local_datapaths. Because pinctrl runs only > > after the engine computation is complete we should have > > complete hmap of all local_datapaths. > > > > Fixes: 647920b6 ("controller: incrementally create ipv6 prefix > delegation port_binding list") > > Reported-at: https://bugzilla.redhat.com/2108726 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > controller/binding.c | 35 ++++++++--------------------------- > > controller/ovn-controller.c | 8 ++++---- > > controller/ovn-controller.h | 6 ------ > > controller/pinctrl.c | 18 ++++++++++-------- > > 4 files changed, 22 insertions(+), 45 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index ba803ae3e..19b28369f 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -509,35 +509,20 @@ static void > > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > struct shash *ras_pd_map) > > { > > - struct pb_ld_binding *ras_pd = > > - shash_find_and_delete(ras_pd_map, pb->logical_port); > > - > > - free(ras_pd); > > + shash_find_and_delete(ras_pd_map, pb->logical_port); > > } > > > > static void > > update_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > - struct hmap *local_datapaths, > > struct shash *map, const char *conf) > > { > > bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); > > struct shash_node *iter = shash_find(map, pb->logical_port); > > - struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; > > > > - if (iter && !ras_pd_conf) { > > + if (!ras_pd_conf && iter) { > > shash_delete(map, iter); > > - free(ras_pd); > > - return; > > - } > > - if (ras_pd_conf) { > > - if (!ras_pd) { > > - ras_pd = xzalloc(sizeof *ras_pd); > > - ras_pd->pb = pb; > > - shash_add(map, pb->logical_port, ras_pd); > > - } > > - ovs_assert(ras_pd); > > - ras_pd->ld = get_local_datapath(local_datapaths, > > - pb->datapath->tunnel_key); > > + } else if (ras_pd_conf && !iter) { > > + shash_add(map, pb->logical_port, pb); > > } > > } > > > > @@ -1735,11 +1720,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > const struct sbrec_port_binding *pb; > > SBREC_PORT_BINDING_TABLE_FOR_EACH (pb, > > b_ctx_in->port_binding_table) { > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ipv6_pd, > > + update_active_pb_ras_pd(pb, > b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ras, > > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > > "ipv6_ra_send_periodic"); > > > > enum en_lport_type lport_type = get_lport_type(pb); > > @@ -2629,12 +2612,10 @@ delete_done: > > continue; > > } > > > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ipv6_pd, > > + update_active_pb_ras_pd(pb, > b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ras, > > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > > "ipv6_ra_send_periodic"); > > > > enum en_lport_type lport_type = get_lport_type(pb); > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 2e9138036..5449743e8 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1312,8 +1312,8 @@ en_runtime_data_cleanup(void *data) > > sset_destroy(&rt_data->egress_ifaces); > > smap_destroy(&rt_data->local_iface_ids); > > local_datapaths_destroy(&rt_data->local_datapaths); > > - shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd); > > - shash_destroy_free_data(&rt_data->local_active_ports_ras); > > + shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > + shash_destroy(&rt_data->local_active_ports_ras); > > local_binding_data_destroy(&rt_data->lbinding_data); > > } > > > > @@ -1425,8 +1425,8 @@ en_runtime_data_run(struct engine_node *node, void > *data) > > first_run = false; > > } else { > > local_datapaths_destroy(local_datapaths); > > - shash_clear_free_data(local_active_ipv6_pd); > > - shash_clear_free_data(local_active_ras); > > + shash_clear(local_active_ipv6_pd); > > + shash_clear(local_active_ras); > > local_binding_data_destroy(&rt_data->lbinding_data); > > sset_destroy(local_lports); > > related_lports_destroy(&rt_data->related_lports); > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > > index df28c62cb..3a0e95377 100644 > > --- a/controller/ovn-controller.h > > +++ b/controller/ovn-controller.h > > @@ -45,10 +45,4 @@ const struct ovsrec_bridge *get_bridge(const struct > ovsrec_bridge_table *, > > > > uint32_t get_tunnel_type(const char *name); > > > > -struct pb_ld_binding { > > - const struct sbrec_port_binding *pb; > > - const struct local_datapath *ld; > > - struct hmap_node hmap_node; > > -}; > > - > > #endif /* controller/ovn-controller.h */ > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 38e8590af..5ea54e7d5 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -1292,18 +1292,20 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > const struct shash *local_active_ports_ipv6_pd, > > const struct sbrec_chassis *chassis, > > - const struct sset *active_tunnels) > > + const struct sset *active_tunnels, > > + const struct hmap *local_datapaths) > > OVS_REQUIRES(pinctrl_mutex) > > { > > bool changed = false; > > > > struct shash_node *iter; > > SHASH_FOR_EACH (iter, local_active_ports_ipv6_pd) { > > - const struct pb_ld_binding *pb_ipv6 = iter->data; > > - const struct sbrec_port_binding *pb = pb_ipv6->pb; > > + const struct sbrec_port_binding *pb = iter->data; > > + const struct local_datapath *ld = > > + get_local_datapath(local_datapaths, > pb->datapath->tunnel_key); > > int j; > > > > - if (!pb_ipv6->ld) { > > + if (!ld) { > > continue; > > } > > > > @@ -1354,7 +1356,7 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > in6_generate_lla(ea, &ip6_addr); > > } > > > > - changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, pb_ipv6->ld, > > + changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ld, > > ea, ip6_addr, > > peer->tunnel_key, > > peer->datapath->tunnel_key); > > @@ -3538,7 +3540,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > prepare_ipv6_ras(local_active_ports_ras, > sbrec_port_binding_by_name); > > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > > local_active_ports_ipv6_pd, chassis, > > - active_tunnels); > > + active_tunnels, local_datapaths); > > sync_dns_cache(dns_table); > > controller_event_run(ovnsb_idl_txn, ce_table, chassis); > > ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths, > > @@ -3982,8 +3984,8 @@ prepare_ipv6_ras(const struct shash > *local_active_ports_ras, > > > > bool changed = false; > > SHASH_FOR_EACH (iter, local_active_ports_ras) { > > - const struct pb_ld_binding *ras = iter->data; > > - const struct sbrec_port_binding *pb = ras->pb; > > + const struct sbrec_port_binding *pb = iter->data; > > + VLOG_INFO("Pop %p", pb); > > I'm guessing you meant to delete this? :) > Oops, yes that shouldn't be there. > > > > > const char *peer_s = smap_get(&pb->options, "peer"); > > if (!peer_s) { > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
