diff mbox series

[ovs-dev,v3,2/2] ovn-controller dns-cache: Improve dns caching using cmaps.

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

Checks

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

Commit Message

Numan Siddique Jan. 13, 2025, 6:34 p.m. UTC
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(-)

Comments

Ales Musil Jan. 14, 2025, 7:02 a.m. UTC | #1
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>
Numan Siddique Jan. 15, 2025, 3:08 a.m. UTC | #2
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 mbox series

Patch

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();
     }