On Tue, Jan 14, 2025 at 2:03 AM Ales Musil <[email protected]> wrote:
>
>
>
> 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]>
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
