On Mon, Jan 6, 2025 at 9:31 AM Ales Musil <[email protected]> wrote:
>
>
>
> On Mon, Dec 16, 2024 at 7:27 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 maintaining 2 hmaps for dns cache and acquiring the
>> lock in the main thread only to swap the active cache to the
>> latest one.
>>
>> Signed-off-by: Numan Siddique <[email protected]>
>> ---
>
>
>
> Hi Numan,
>
> would it work in your case if you would replace hmap with cmap
> for the cache? That should effectively get rid of the lock completely
> and will have the same effect that you might get "outdated" version
> in the pinctrl if something is writing into it.
Thanks for the comments. Your idea makes sense. I'm working on using cmap
and I'll submit v2 with these changes.
Thanks
Numan
>
>> controller/ovn-dns.c | 94 +++++++++++++++++++++++---------------------
>> 1 file changed, 50 insertions(+), 44 deletions(-)
>>
>> diff --git a/controller/ovn-dns.c b/controller/ovn-dns.c
>> index 026d9bc64..cf384b043 100644
>> --- a/controller/ovn-dns.c
>> +++ b/controller/ovn-dns.c
>> @@ -38,7 +38,9 @@ struct dns_data {
>> };
>>
>> /* shash of 'struct dns_data'. */
>> -static struct hmap dns_cache_ = HMAP_INITIALIZER(&dns_cache_);
>> +static struct hmap dns_caches[2];
>> +static struct hmap *active_dns_cache_;
>> +static uint8_t active_dns_cache_idx;
>>
>> /* Mutex to protect dns_cache_. */
>> static struct ovs_mutex dns_cache_mutex = OVS_MUTEX_INITIALIZER;
>> @@ -47,58 +49,49 @@ 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);
>> +static struct dns_data *dns_data_find(const struct uuid *uuid,
>> + const struct hmap *);
>> 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 hmap *dns_cache);
>>
>> void
>> ovn_dns_cache_init(void)
>> {
>> + hmap_init(&dns_caches[0]);
>> + hmap_init(&dns_caches[1]);
>> + active_dns_cache_idx = 0;
>> + active_dns_cache_ = &dns_caches[0];
>> }
>>
>> 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_);
>> + destroy_dns_cache(&dns_caches[0]);
>> + destroy_dns_cache(&dns_caches[1]);
>> + hmap_destroy(&dns_caches[0]);
>> + hmap_destroy(&dns_caches[1]);
>> + active_dns_cache_ = NULL;
>> ovs_mutex_unlock(&dns_cache_mutex);
>> }
>>
>> void
>> ovn_dns_sync_cache(const struct sbrec_dns_table *dns_table)
>> {
>> - ovs_mutex_lock(&dns_cache_mutex);
>> + const struct sbrec_dns *sbrec_dns;
>> struct dns_data *dns_data;
>> - HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache_) {
>> - dns_data->delete = true;
>> - }
>>
>> - const struct sbrec_dns *sbrec_dns;
>> + uint8_t next_cache_idx = (active_dns_cache_idx + 1) % 2;
>> + struct hmap *dns_cache = &dns_caches[next_cache_idx];
>> + ovs_assert(dns_cache != active_dns_cache_);
>> +
>> 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 = dns_data_alloc(*uuid);
>> + hmap_insert(dns_cache, &dns_data->hmap_node, uuid_hash(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));
>> @@ -107,30 +100,33 @@ ovn_dns_sync_cache(const struct sbrec_dns_table
>> *dns_table)
>> }
>> }
>>
>> - 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);
>> - }
>> - }
>> + struct hmap *prev_active_cache = active_dns_cache_;
>> +
>> + ovs_mutex_lock(&dns_cache_mutex);
>> + active_dns_cache_ = dns_cache;
>> + active_dns_cache_idx = next_cache_idx;
>> ovs_mutex_unlock(&dns_cache_mutex);
>> +
>> + /* Destroy the previous active dns cache. */
>> + destroy_dns_cache(prev_active_cache);
>> }
>>
>> void
>> ovn_dns_update_cache(const struct sbrec_dns_table *dns_table)
>> {
>> ovs_mutex_lock(&dns_cache_mutex);
>> -
>> + ovs_assert(active_dns_cache_ != NULL);
>> const struct sbrec_dns *sbrec_dns;
>> 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);
>> + struct dns_data *dns_data = dns_data_find(uuid, active_dns_cache_);
>>
>> if (sbrec_dns_is_deleted(sbrec_dns) && dns_data) {
>> - hmap_remove(&dns_cache_, &dns_data->hmap_node);
>> + hmap_remove(active_dns_cache_, &dns_data->hmap_node);
>> dns_data_destroy(dns_data);
>> } else {
>> - update_cache_with_dns_rec(sbrec_dns, dns_data, uuid,
>> &dns_cache_);
>> + update_cache_with_dns_rec(sbrec_dns, dns_data, uuid,
>> + active_dns_cache_);
>> }
>> }
>>
>> @@ -142,10 +138,11 @@ ovn_dns_lookup(const char *query_name, uint64_t
>> dp_key, bool *ovn_owned)
>> {
>> ovs_mutex_lock(&dns_cache_mutex);
>>
>> + ovs_assert(active_dns_cache_ != NULL);
>> *ovn_owned = false;
>> struct dns_data *dns_data;
>> const char *answer_data = NULL;
>> - HMAP_FOR_EACH (dns_data, hmap_node, &dns_cache_) {
>> + HMAP_FOR_EACH (dns_data, hmap_node, active_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
>> @@ -205,11 +202,11 @@ update_cache_with_dns_rec(const struct sbrec_dns
>> *sbrec_dns,
>> }
>>
>> static struct dns_data *
>> -dns_data_find(const struct uuid *uuid)
>> +dns_data_find(const struct uuid *uuid, const struct hmap *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_) {
>> + HMAP_FOR_EACH_WITH_HASH (dns_data, hmap_node, hash, dns_cache) {
>> if (uuid_equals(&dns_data->uuid, uuid)) {
>> return dns_data;
>> }
>> @@ -240,3 +237,12 @@ dns_data_destroy(struct dns_data *dns_data)
>> free(dns_data->dps);
>> free(dns_data);
>> }
>> +
>> +static void
>> +destroy_dns_cache(struct hmap *dns_cache)
>> +{
>> + struct dns_data *dns_data;
>> + HMAP_FOR_EACH_POP (dns_data, hmap_node, dns_cache) {
>> + dns_data_destroy(dns_data);
>> + }
>> +}
>> --
>> 2.39.3 (Apple Git-146)
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> Thanks,
> Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
