[v6,0/5] Address reader-writer concurrency in rte_hash

Message ID 1540344746-29045-1-git-send-email-honnappa.nagarahalli@arm.com
Headers show
Series
  • Address reader-writer concurrency in rte_hash
Related show

Message

Honnappa Nagarahalli Oct. 24, 2018, 1:32 a.m.
This patch has dependency on the following patches in the order:
    http://patchwork.dpdk.org/cover/45611/
    http://patchwork.dpdk.org/patch/47196/

    Currently, reader-writer concurrency problems in rte_hash are
    addressed using reader-writer locks. Use of reader-writer locks
    results in following issues:
    
    1) In many of the use cases for the hash table, writer threads
       are running on control plane. If the writer is preempted while
       holding the lock, it will block the readers for an extended period
       resulting in packet drops. This problem seems to apply for platforms
       with transactional memory support as well because of the algorithm
       used for rte_rwlock_write_lock_tm:
    
       static inline void
       rte_rwlock_write_lock_tm(rte_rwlock_t *rwl)
       {
            if (likely(rte_try_tm(&rwl->cnt)))
                    return;
            rte_rwlock_write_lock(rwl);
       }
    
       i.e. there is a posibility of using rte_rwlock_write_lock in
       failure cases.
    2) Reader-writer lock based solution does not address the following
       issue.
       rte_hash_lookup_xxx APIs return the index of the element in
       the key store. Application(reader) can use that index to reference
       other data structures in its scope. Because of this, the
       index should not be freed till the application completes
       using the index.
    3) Since writer blocks all the readers, the hash lookup
       rate comes down significantly when there is activity on the writer.
       This happens even for unrelated entries. Performance numbers
       given below clearly indicate this.
    
    Lock-free solution is required to solve these problems. This patch
    series adds the lock-free capabilities in the following steps:

    1) Add support to not free the key-store index upon calling
       rte_hash_del_xxx APIs. This solves the issue in 2).
    
    2) Correct the alignment for the key store entry to prep for
       memory ordering.

    3) Add memory ordering to prevent race conditions when a new key
       is added to the table.
    
    4) Reader-writer concurrency issue, caused by moving the keys
       to their alternate locations during key insert, is solved
       by introducing an atomic global counter indicating a change
       in table.
    
    5) This solution also has to solve the issue of readers using
       key store element even after the key is deleted from
       control plane.
       To solve this issue, the hash_del_key_xxx APIs do not free
       the key store element when lock-free algorithm is enabled.
       The key store element has to be freed using the newly introduced
       rte_hash_free_key_with_position API. It needs to be called once
       all the readers have stopped using the key store element. How this
       is determined is outside the scope of this patch (RCU is one such
       mechanism that the application can use).
    
    6) Finally, a lock free reader-writer concurrency flag is added
       to enable this feature at run time.
    
    Performance numbers can be got from the additional test case added
    as part of this patch.

    v5->v6
    1) Added lock-free RW concurrency tests to autotest_data.py and
       meson test
    2) Fixed Yipeng's email ID
    3) Added Bruce's Acked-by

    v4->v5
    1) Rebased with patch v8 of extendable hash bucket feature
       (http://patchwork.dpdk.org/patch/47196/)
    2) Changed 'success' to 'hit' and 'fail' to 'miss' in read/write
       concurrenct lock free test code

    v3-v4
    1) Merged 4/7, 5/7 and 6/7 into 4/5
    2) Changed RTE_HASH_EXTRA_FLAGS_RECYCLE_ON_DEL to
       RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL (Yipeng)
    3) Changed the commit log for the patch
       "hash: correct key store element alignment" (Yipeng)
    4) Changed the comment for rte_hash_add_key_data API (Yipeng)
    5) Added bulk lookup for lock-free performance test case (Yipeng)
    5) Reduced the number of keys to 4M in the tests (Yipeng)

    v2->v3
    1) Rebased on top of:
       http://patchwork.dpdk.org/cover/45611/
       http://patchwork.dpdk.org/project/dpdk/list/?series=1822
    2) Added comments to RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF
       to indicate multi writer support (Yipeng)
    3) Updated the comments for rte_hash_add_key_data_xxx APIs
       to free the 'data' only after the readers have completed
       using 'data' (Yipeng)
    4) Extendable tables are not supported when lock free algorithm
       is requested.

    v1->v2
    1) Separate multi-writer capability from rw concurrency
    2) Add do not recycle on delete feature (Yipeng)
    3) Add Arm copyright
    4) Add test case to test lock-free algorithm and multi-writer
       test case (Yipeng)
    5) Additional API documentation to indicate RCU usage (Yipeng)
    6) Additional documentation on rte_hash_reset API (Yipeng)
    7) Allocate memory for the global counter and avoid API
       changes (Yipeng)

Dharmik Thakkar (1):
  test/hash: read-write lock-free concurrency test

Honnappa Nagarahalli (4):
  hash: separate multi-writer from rw-concurrency
  hash: support do not free on delete
  hash: fix key store element alignment
  hash: add lock-free read-write concurrency

 lib/librte_hash/rte_cuckoo_hash.c    |  520 +++++++++++----
 lib/librte_hash/rte_cuckoo_hash.h    |   21 +-
 lib/librte_hash/rte_hash.h           |   77 ++-
 lib/librte_hash/rte_hash_version.map |    7 +
 test/test/Makefile                   |    1 +
 test/test/autotest_data.py           |    6 +
 test/test/meson.build                |    2 +
 test/test/test_hash.c                |  140 +++-
 test/test/test_hash_readwrite.c      |    6 +-
 test/test/test_hash_readwrite_lf.c   | 1220 ++++++++++++++++++++++++++++++++++
 10 files changed, 1855 insertions(+), 145 deletions(-)
 create mode 100644 test/test/test_hash_readwrite_lf.c

Comments

Thomas Monjalon Oct. 25, 2018, 10:56 p.m. | #1
24/10/2018 03:32, Honnappa Nagarahalli:
> Dharmik Thakkar (1):
>   test/hash: read-write lock-free concurrency test
> 
> Honnappa Nagarahalli (4):
>   hash: separate multi-writer from rw-concurrency
>   hash: support do not free on delete
>   hash: fix key store element alignment
>   hash: add lock-free read-write concurrency

Please add Cc: stable@dpdk.org below Fixes: line
when the fix must be backported.

The script check-git-log.sh will help in this task.
Honnappa Nagarahalli Oct. 26, 2018, 12:48 a.m. | #2
> 
> 24/10/2018 03:32, Honnappa Nagarahalli:
> > Dharmik Thakkar (1):
> >   test/hash: read-write lock-free concurrency test
> >
> > Honnappa Nagarahalli (4):
> >   hash: separate multi-writer from rw-concurrency
> >   hash: support do not free on delete
> >   hash: fix key store element alignment
> >   hash: add lock-free read-write concurrency
> 
> Please add Cc: stable@dpdk.org below Fixes: line when the fix must be
> backported.
> 
> The script check-git-log.sh will help in this task.
I did not run this script, will run from next time.

I am confused about when to add 'stable@dpdk.org'.
https://doc.dpdk.org/guides-18.08/contributing/patches.html  section 5.7 says that while fixing a 'regression' (any issue due to a previous code change), we should add 'author@example.com'.
Then section 5.7.2 says that 'all fix patches to the master that are candidates for backporting should have 'Cc:stable@dpdk.org' and should be CCed to stable@dpdk.org mailing list.
Last time I CCed a patch to stable@dpdk.org, I was told not to do it, 'Cc:stable@dpdk.org' in the commit message is enough.

Any clarification would help.

>
Thomas Monjalon Oct. 26, 2018, 8:38 a.m. | #3
26/10/2018 02:48, Honnappa Nagarahalli:
> > 
> > 24/10/2018 03:32, Honnappa Nagarahalli:
> > > Dharmik Thakkar (1):
> > >   test/hash: read-write lock-free concurrency test
> > >
> > > Honnappa Nagarahalli (4):
> > >   hash: separate multi-writer from rw-concurrency
> > >   hash: support do not free on delete
> > >   hash: fix key store element alignment
> > >   hash: add lock-free read-write concurrency
> > 
> > Please add Cc: stable@dpdk.org below Fixes: line when the fix must be
> > backported.
> > 
> > The script check-git-log.sh will help in this task.
> I did not run this script, will run from next time.
> 
> I am confused about when to add 'stable@dpdk.org'.
> https://doc.dpdk.org/guides-18.08/contributing/patches.html  section 5.7 says that while fixing a 'regression' (any issue due to a previous code change), we should add 'author@example.com'.
> Then section 5.7.2 says that 'all fix patches to the master that are candidates for backporting should have 'Cc:stable@dpdk.org' and should be CCed to stable@dpdk.org mailing list.
> Last time I CCed a patch to stable@dpdk.org, I was told not to do it, 'Cc:stable@dpdk.org' in the commit message is enough.
> 
> Any clarification would help.

If you add Cc: stable@dpdk.org in the commit, git-send-email will
automatically Cc the email. So the email will be sent to dev and stable
at the same time. It's what should happen :)