On Mon, Jan 13, 2025 at 7:35 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. > > Since cmaps use OVSRCU_TYPE, in order to safely free the cmap data, > we need to manage the quiesce state for both the main ovn-controller > thread and pinctrl thread. ovsrcu_quiesce_start() is called before > calling poll_block() and ovsrcu_quiesce_end() is called at the > start of the while for both these threads. > > Suggested-by: Ales Musil <[email protected]> > Signed-off-by: Numan Siddique <[email protected]> > --- > controller/ovn-controller.c | 3 + > controller/ovn-dns.c | 144 +++++++++++++++--------------------- > controller/pinctrl.c | 4 + > 3 files changed, 67 insertions(+), 84 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index d875ecc1e6..890c8f988b 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5458,6 +5458,8 @@ main(int argc, char *argv[]) > /* Main loop. */ > bool sb_monitor_all = false; > while (!exit_args.exiting) { > + ovsrcu_quiesce_end(); > + > memory_run(); > if (memory_should_report()) { > struct simap usage = SIMAP_INITIALIZER(&usage); > @@ -5951,6 +5953,7 @@ main(int argc, char *argv[]) > > loop_done: > memory_wait(); > + ovsrcu_quiesce_start(); > poll_block(); > if (should_service_stop()) { > exit_args.exiting = true; > diff --git a/controller/ovn-dns.c b/controller/ovn-dns.c > index 026d9bc647..bfeef5a747 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)); > + ovsrcu_postpone(dns_data_destroy, existing); > } > } > - 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)); > + ovsrcu_postpone(dns_data_destroy, existing); > } 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)); > + ovsrcu_postpone(dns_data_destroy, existing); > + } > } > > 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) { > + ovsrcu_postpone(dns_data_destroy, dns_data); > + } > +} > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index e1bae61515..eaa0985021 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3902,6 +3902,8 @@ pinctrl_handler(void *arg_) > static long long int send_prefixd_time = LLONG_MAX; > > while (!latch_is_set(&pctrl->pinctrl_thread_exit)) { > + ovsrcu_quiesce_end(); > + > long long int bfd_time = LLONG_MAX; > bool lock_failed = false; > > @@ -3975,6 +3977,8 @@ pinctrl_handler(void *arg_) > > latch_wait(&pctrl->pinctrl_thread_exit); > } > + > + ovsrcu_quiesce_start(); > poll_block(); > } > > -- > 2.47.1 > > Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
