DPDK patches and discussions
 help / color / mirror / Atom feed
From: bugzilla@dpdk.org
To: dev@dpdk.org
Subject: [dpdk-dev] [Bug 261] DPDK 18.11 bug on rte_hash_free_key_with_position
Date: Tue, 30 Apr 2019 09:03:58 +0000	[thread overview]
Message-ID: <bug-261-3@http.bugs.dpdk.org/> (raw)
Message-ID: <20190430090358.nnWzPm7m1M4ZLsNN9xDeq9pkXcF9EwGalw8mwdL2aOA@z> (raw)

https://bugs.dpdk.org/show_bug.cgi?id=261

            Bug ID: 261
           Summary: DPDK 18.11 bug on rte_hash_free_key_with_position
           Product: DPDK
           Version: 18.11
          Hardware: All
                OS: All
            Status: CONFIRMED
          Severity: normal
          Priority: Normal
         Component: other
          Assignee: dev@dpdk.org
          Reporter: zhongdahulinfan@163.com
  Target Milestone: ---

First let's see the definition of rte_hash_free_key_with_position in DPDK
18.11, as shown bellow: 

int __rte_experimental
rte_hash_free_key_with_position(const struct rte_hash *h,
                const int32_t position)
{
    RETURN_IF_TRUE(((h == NULL) || (position == EMPTY_SLOT)), -EINVAL);

    unsigned int lcore_id, n_slots;
    struct lcore_cache *cached_free_slots;
    const int32_t total_entries = h->num_buckets * RTE_HASH_BUCKET_ENTRIES;

    /* Out of bounds */
    if (position >= total_entries)
        return -EINVAL;

    if (h->use_local_cache) {
        lcore_id = rte_lcore_id();
        cached_free_slots = &h->local_free_slots[lcore_id];
        /* Cache full, need to free it. */
        if (cached_free_slots->len == LCORE_CACHE_SIZE) {
            /* Need to enqueue the free slots in global ring. */
            n_slots = rte_ring_mp_enqueue_burst(h->free_slots,
                        cached_free_slots->objs,
                        LCORE_CACHE_SIZE, NULL);
            cached_free_slots->len -= n_slots;
        }
        /* Put index of new free slot in cache. */
        cached_free_slots->objs[cached_free_slots->len] =
                    (void *)((uintptr_t)position);
        cached_free_slots->len++;
    } else {
        rte_ring_sp_enqueue(h->free_slots,
                (void *)((uintptr_t)position));
    }

    return 0;
}

There are two issues for this API.

First, the input parameter 'position' is the key index of the hash table, which
is returned by rte_hash_add_key_xxx or rte_hash_del_key_xxx. Take a glance look
of rte_hash_del_key_with_hash for example, we see that it returns key_idx - 1
if entry found and removed successfully. Hence rte_hash_free_key_with_position
is not correct while it enqueues position into free_slots directly. It must
increase position by one to get the right key index, before enqueues into
free_slots.

As comparision, remove_entry()enqueue key_idx directly, which is correct: 

static inline void
remove_entry(const struct rte_hash *h, struct rte_hash_bucket *bkt, unsigned i)
{
        unsigned lcore_id, n_slots;
        struct lcore_cache *cached_free_slots;

        if (h->use_local_cache) {
                lcore_id = rte_lcore_id();
                cached_free_slots = &h->local_free_slots[lcore_id];
                /* Cache full, need to free it. */
                if (cached_free_slots->len == LCORE_CACHE_SIZE) {
                        /* Need to enqueue the free slots in global ring. */
                        n_slots = rte_ring_mp_enqueue_burst(h->free_slots,
                                                cached_free_slots->objs,
                                                LCORE_CACHE_SIZE, NULL);
                        cached_free_slots->len -= n_slots;
                }
                /* Put index of new free slot in cache. */
                cached_free_slots->objs[cached_free_slots->len] =
                                (void *)((uintptr_t)bkt->key_idx[i]);
                cached_free_slots->len++;
        } else {
                rte_ring_sp_enqueue(h->free_slots,
                                (void *)((uintptr_t)bkt->key_idx[i]));
        }
}

Second, computation of total_entries is not correct. This should be the total
number of key slots.The number of key slots is seen as rte_hash_create, say
(params->entries + (RTE_MAX_LCORE - 1) *(LCORE_CACHE_SIZE - 1) + 1) when
use_local_cache is true, else (params->entries + 1)

struct rte_hash *
rte_hash_create(const struct rte_hash_parameters *params)
{
...
    if (params->extra_flag & RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD) {
        use_local_cache = 1;
        writer_takes_lock = 1;
    }
...
    /* Store all keys and leave the first entry as a dummy entry for
lookup_bulk */
    if (use_local_cache)
        /*
         * Increase number of slots by total number of indices
         * that can be stored in the lcore caches
         * except for the first cache
         */
        num_key_slots = params->entries + (RTE_MAX_LCORE - 1) *
                    (LCORE_CACHE_SIZE - 1) + 1;
    else
        num_key_slots = params->entries + 1;
...
    /* Populate free slots ring. Entry zero is reserved for key misses. */
    for (i = 1; i < num_key_slots; i++)
        rte_ring_sp_enqueue(r, (void *)((uintptr_t) i));
...
}

-- 
You are receiving this mail because:
You are the assignee for the bug.

             reply	other threads:[~2019-04-30  9:04 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-30  9:03 bugzilla [this message]
2019-04-30  9:03 ` bugzilla
2019-05-01  3:33 ` Dharmik Thakkar
2019-05-01  3:33   ` Dharmik Thakkar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bug-261-3@http.bugs.dpdk.org/ \
    --to=bugzilla@dpdk.org \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).