Message ID | 20250113183444.356616-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v3,1/2] pinctrl: Use ovs_mutex_trylock() in the pinctrl thread. | 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 | fail | github build: failed |
On Mon, Jan 13, 2025 at 7:35 PM <numans@ovn.org> wrote: > From: Numan Siddique <numans@ovn.org> > > 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 <amusil@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > 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 <amusil@redhat.com>
On Tue, Jan 14, 2025 at 2:03 AM Ales Musil <amusil@redhat.com> wrote: > > > > On Mon, Jan 13, 2025 at 7:35 PM <numans@ovn.org> wrote: >> >> From: Numan Siddique <numans@ovn.org> >> >> 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 <amusil@redhat.com> >> Signed-off-by: Numan Siddique <numans@ovn.org> >> --- >> 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 <amusil@redhat.com> Thanks Ales for the review. I applied this patch series to main and also backported to branch-24.09. I also had to backport the below 2 prereq patches: 817d4e53e8 ("ovn-controller: Add a separate dns cache module and I-P for SB DNS.") 1270d71762 ("northd: Use the same UUID for SB representation of DNS."). Numan
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(); }