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.
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
