On Mon, Jan 13, 2025 at 5:11 AM Ales Musil <[email protected]> wrote: > > > > On Thu, Jan 9, 2025 at 10:57 PM <[email protected]> wrote: >> >> From: Numan Siddique <[email protected]> >> >> Its possible for pinctrl handling for DNS packets to be blocked >> if the ovn-controller main thread has acquired the dns cache >> mutex during the recompute. This patch improves the DNS packet >> handling by using cmap instead of hmap to maintain the dns cache. >> With this, there is no need to maintain a mutex. >> >> Suggested-by: Ales Musil <[email protected]> >> Signed-off-by: Numan Siddique <[email protected]> >> --- > > > Hi Numan, > > thank you for the follow up, I have a few comments down below.
Thanks Ales for pointing out the right way to use maps. I've addressed your comments in v3. Please check it out - https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Numan > > >> controller/ovn-dns.c | 144 ++++++++++++++++++------------------------- >> 1 file changed, 60 insertions(+), 84 deletions(-) >> >> diff --git a/controller/ovn-dns.c b/controller/ovn-dns.c >> index 026d9bc647..d488a7d8e4 100644 >> --- a/controller/ovn-dns.c >> +++ b/controller/ovn-dns.c >> @@ -18,6 +18,7 @@ >> /* OVS includes */ >> #include "include/openvswitch/shash.h" >> #include "include/openvswitch/thread.h" >> +#include "lib/cmap.h" >> #include "openvswitch/vlog.h" >> >> /* OVN includes. */ >> @@ -28,7 +29,7 @@ VLOG_DEFINE_THIS_MODULE(ovndns); >> >> /* Internal DNS cache entry for each SB DNS record. */ >> struct dns_data { >> - struct hmap_node hmap_node; >> + struct cmap_node cmap_node; >> struct uuid uuid; >> uint64_t *dps; >> size_t n_dps; >> @@ -38,114 +39,87 @@ struct dns_data { >> }; >> >> /* shash of 'struct dns_data'. */ >> -static struct hmap dns_cache_ = HMAP_INITIALIZER(&dns_cache_); >> - >> -/* Mutex to protect dns_cache_. */ >> -static struct ovs_mutex dns_cache_mutex = OVS_MUTEX_INITIALIZER; >> +static struct cmap dns_cache_; >> >> static void update_cache_with_dns_rec(const struct sbrec_dns *, >> struct dns_data *, >> const struct uuid *uuid, >> - struct hmap *dns_cache); >> -static struct dns_data *dns_data_find(const struct uuid *uuid); >> + struct cmap *dns_cache); >> +static struct dns_data *dns_data_find(const struct uuid *uuid, >> + const struct cmap *); >> static struct dns_data *dns_data_alloc(struct uuid uuid); >> static void dns_data_destroy(struct dns_data *dns_data); >> +static void destroy_dns_cache(struct cmap *dns_cache); >> >> void >> ovn_dns_cache_init(void) >> { >> + cmap_init(&dns_cache_); >> } >> >> void >> ovn_dns_cache_destroy(void) >> { >> - ovs_mutex_lock(&dns_cache_mutex); >> - struct dns_data *dns_data; >> - HMAP_FOR_EACH_POP (dns_data, hmap_node, &dns_cache_) { >> - dns_data_destroy(dns_data); >> - } >> - hmap_destroy(&dns_cache_); >> - ovs_mutex_unlock(&dns_cache_mutex); >> + destroy_dns_cache(&dns_cache_); >> + cmap_destroy(&dns_cache_); >> } >> >> void >> ovn_dns_sync_cache(const struct sbrec_dns_table *dns_table) >> { >> - ovs_mutex_lock(&dns_cache_mutex); >> - struct dns_data *dns_data; >> - HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache_) { >> - dns_data->delete = true; >> + const struct sbrec_dns *sbrec_dns; >> + struct dns_data *existing; >> + >> + CMAP_FOR_EACH (existing, cmap_node, &dns_cache_) { >> + existing->delete = true; >> } >> >> - const struct sbrec_dns *sbrec_dns; >> SBREC_DNS_TABLE_FOR_EACH (sbrec_dns, dns_table) { >> const struct uuid *uuid = &sbrec_dns->header_.uuid; >> - dns_data = dns_data_find(uuid); >> - if (!dns_data) { >> - dns_data = dns_data_alloc(*uuid); >> - hmap_insert(&dns_cache_, &dns_data->hmap_node, uuid_hash(uuid)); >> - } else { >> - free(dns_data->dps); >> - } >> - >> - dns_data->delete = false; >> - >> - if (!smap_equal(&dns_data->records, &sbrec_dns->records)) { >> - smap_destroy(&dns_data->records); >> - smap_clone(&dns_data->records, &sbrec_dns->records); >> - } >> - >> - if (!smap_equal(&dns_data->options, &sbrec_dns->options)) { >> - smap_destroy(&dns_data->options); >> - smap_clone(&dns_data->options, &sbrec_dns->options); >> - } >> - >> - dns_data->n_dps = sbrec_dns->n_datapaths; >> - dns_data->dps = xcalloc(dns_data->n_dps, sizeof(uint64_t)); >> - for (size_t i = 0; i < sbrec_dns->n_datapaths; i++) { >> - dns_data->dps[i] = sbrec_dns->datapaths[i]->tunnel_key; >> - } >> + existing = dns_data_find(uuid, &dns_cache_); >> + update_cache_with_dns_rec(sbrec_dns, existing, uuid, >> + &dns_cache_); >> } >> >> - HMAP_FOR_EACH_SAFE (dns_data, hmap_node, &dns_cache_) { >> - if (dns_data->delete) { >> - hmap_remove(&dns_cache_, &dns_data->hmap_node); >> - dns_data_destroy(dns_data); >> + CMAP_FOR_EACH (existing, cmap_node, &dns_cache_) { >> + if (existing->delete) { >> + cmap_remove(&dns_cache_, &existing->cmap_node, >> + uuid_hash(&existing->uuid)); >> >> + dns_data_destroy(existing); > > > It is not safe to call "dns_data_destroy()" right away as > the pinctrl might actually hold a pointer to one of the records. > The proper way to do it is to use > "ovsrcu_postpone(dns_data_destroy, existing)". This has the downside > that we need to manage the quiesce state for the RCU. > > The call to "ovsrcu_quiesce_start()" should go before "poll_block()" > for both ovn-controller and pinctrl thread. That indicates that both > threads are done with the data and it's safe to free them. The > opposite indication "ovsrcu_quiesce_end()" should be the first call > in the while loop for both threads. > > By setting this up we don't have to care about this state even if > we decide to add more cmaps into pinctrl later on. > >> >> } >> } >> - ovs_mutex_unlock(&dns_cache_mutex); >> } >> >> void >> ovn_dns_update_cache(const struct sbrec_dns_table *dns_table) >> { >> - ovs_mutex_lock(&dns_cache_mutex); >> - >> const struct sbrec_dns *sbrec_dns; >> + struct dns_data *existing; >> + >> SBREC_DNS_TABLE_FOR_EACH_TRACKED (sbrec_dns, dns_table) { >> const struct uuid *uuid = &sbrec_dns->header_.uuid; >> - struct dns_data *dns_data = dns_data_find(uuid); >> >> - if (sbrec_dns_is_deleted(sbrec_dns) && dns_data) { >> - hmap_remove(&dns_cache_, &dns_data->hmap_node); >> - dns_data_destroy(dns_data); >> + existing = dns_data_find(uuid, &dns_cache_); >> + if (sbrec_dns_is_deleted(sbrec_dns) && existing) { >> + cmap_remove(&dns_cache_, &existing->cmap_node, >> + uuid_hash(&existing->uuid)); >> + dns_data_destroy(existing); > > > It should be postponed here too. > >> } else { >> - update_cache_with_dns_rec(sbrec_dns, dns_data, uuid, >> &dns_cache_); >> + update_cache_with_dns_rec(sbrec_dns, existing, uuid, >> + &dns_cache_); >> } >> } >> - >> - ovs_mutex_unlock(&dns_cache_mutex); >> } >> >> const char * >> ovn_dns_lookup(const char *query_name, uint64_t dp_key, bool *ovn_owned) >> { >> - ovs_mutex_lock(&dns_cache_mutex); >> + const char *answer_data = NULL; >> + struct dns_data *dns_data; >> >> *ovn_owned = false; >> - struct dns_data *dns_data; >> - const char *answer_data = NULL; >> - HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache_) { >> + >> + CMAP_FOR_EACH (dns_data, cmap_node, &dns_cache_) { >> for (size_t i = 0; i < dns_data->n_dps; i++) { >> if (dns_data->dps[i] == dp_key) { >> /* DNS records in SBDB are stored in lowercase. Convert to >> @@ -167,8 +141,6 @@ ovn_dns_lookup(const char *query_name, uint64_t dp_key, >> bool *ovn_owned) >> } >> } >> >> - ovs_mutex_unlock(&dns_cache_mutex); >> - >> return answer_data; >> } >> >> @@ -176,40 +148,35 @@ ovn_dns_lookup(const char *query_name, uint64_t >> dp_key, bool *ovn_owned) >> /* Static functions. */ >> static void >> update_cache_with_dns_rec(const struct sbrec_dns *sbrec_dns, >> - struct dns_data *dns_data, >> + struct dns_data *existing, >> const struct uuid *uuid, >> - struct hmap *dns_cache) >> + struct cmap *dns_cache) >> { >> - if (!dns_data) { >> - dns_data = dns_data_alloc(*uuid); >> - hmap_insert(dns_cache, &dns_data->hmap_node, uuid_hash(uuid)); >> - } else { >> - free(dns_data->dps); >> - } >> - >> - if (!smap_equal(&dns_data->records, &sbrec_dns->records)) { >> - smap_destroy(&dns_data->records); >> - smap_clone(&dns_data->records, &sbrec_dns->records); >> - } >> - >> - if (!smap_equal(&dns_data->options, &sbrec_dns->options)) { >> - smap_destroy(&dns_data->options); >> - smap_clone(&dns_data->options, &sbrec_dns->options); >> - } >> + struct dns_data *dns_data = dns_data_alloc(*uuid); >> + smap_clone(&dns_data->records, &sbrec_dns->records); >> + smap_clone(&dns_data->options, &sbrec_dns->options); >> >> dns_data->n_dps = sbrec_dns->n_datapaths; >> dns_data->dps = xcalloc(dns_data->n_dps, sizeof(uint64_t)); >> for (size_t i = 0; i < sbrec_dns->n_datapaths; i++) { >> dns_data->dps[i] = sbrec_dns->datapaths[i]->tunnel_key; >> } >> + >> + if (!existing) { >> + cmap_insert(dns_cache, &dns_data->cmap_node, uuid_hash(uuid)); >> + } else { >> + cmap_replace(dns_cache, &existing->cmap_node, &dns_data->cmap_node, >> + uuid_hash(uuid)); >> + dns_data_destroy(existing); > > > Same as above. > >> + } >> } >> >> static struct dns_data * >> -dns_data_find(const struct uuid *uuid) >> +dns_data_find(const struct uuid *uuid, const struct cmap *dns_cache) >> { >> struct dns_data *dns_data; >> size_t hash = uuid_hash(uuid); >> - HMAP_FOR_EACH_WITH_HASH (dns_data, hmap_node, hash, &dns_cache_) { >> + CMAP_FOR_EACH_WITH_HASH (dns_data, cmap_node, hash, dns_cache) { >> if (uuid_equals(&dns_data->uuid, uuid)) { >> return dns_data; >> } >> @@ -240,3 +207,12 @@ dns_data_destroy(struct dns_data *dns_data) >> free(dns_data->dps); >> free(dns_data); >> } >> + >> +static void >> +destroy_dns_cache(struct cmap *dns_cache) >> +{ >> + struct dns_data *dns_data; >> + CMAP_FOR_EACH (dns_data, cmap_node, dns_cache) { >> + dns_data_destroy(dns_data); > > > Same as above. > >> >> + } >> +} >> -- >> 2.47.1 >> > > Thanks, > Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
