To flush a ring not in use, dequeue one by one is wasting cpu cycles. The patch is to just resetting the head and tail indices to save cpu cycle. Gavin Hu (2): ring: add rte_ring_reset api to flush the ring hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 4 files changed, 32 insertions(+), 8 deletions(-) -- 2.11.0
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <Ruifeng.Wang@arm.com> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> --- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index af5444a9f..283030011 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +static inline void __rte_experimental +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd0d..581d9cabe 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.11.0
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435dfd..5669d83f4 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index c55a4f263..2719af3ce 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -559,7 +559,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -570,16 +569,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.11.0
V2: fix the coding style issue(commit message line too long) V1: To flush a ring not in use, dequeue one by one is wasting cpu cycles. The patch is to just resetting the head and tail indices to save cpu cycle. Gavin Hu (2): ring: add reset api to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 4 files changed, 32 insertions(+), 8 deletions(-) -- 2.11.0
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <Ruifeng.Wang@arm.com> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> --- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index af5444a9f..283030011 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +static inline void __rte_experimental +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd0d..581d9cabe 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.11.0
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435dfd..5669d83f4 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index c55a4f263..2719af3ce 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -559,7 +559,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -570,16 +569,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.11.0
On Wed, Dec 12, 2018 at 02:47:33PM +0800, Gavin Hu wrote:
> Within rte_hash_reset, calling a while loop to dequeue one by
> one from the ring, while not using them at all, is wasting cycles,
> The patch just flush the ring by resetting the indices can save cpu
> cycles.
>
> Fixes: b26473ff8f4a ("hash: add reset function")
> Fixes: 75706568a7eb ("hash: add extendable bucket feature")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> ---
> lib/librte_hash/Makefile | 2 +-
> lib/librte_hash/rte_cuckoo_hash.c | 11 ++++-------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile
> index c8c435dfd..5669d83f4 100644
> --- a/lib/librte_hash/Makefile
> +++ b/lib/librte_hash/Makefile
> @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> # library name
> LIB = librte_hash.a
>
> -CFLAGS += -O3
> +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API
> CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR)
> LDLIBS += -lrte_eal -lrte_ring
>
Is a similar change needed to meson.build?
On 2018-12-12 07:47, Gavin Hu wrote:
> Within rte_hash_reset, calling a while loop to dequeue one by
> one from the ring, while not using them at all, is wasting cycles,
> The patch just flush the ring by resetting the indices can save cpu
> cycles.
>
> Fixes: b26473ff8f4a ("hash: add reset function")
> Fixes: 75706568a7eb ("hash: add extendable bucket feature")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> ---
> lib/librte_hash/Makefile | 2 +-
> lib/librte_hash/rte_cuckoo_hash.c | 11 ++++-------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile
> index c8c435dfd..5669d83f4 100644
> --- a/lib/librte_hash/Makefile
> +++ b/lib/librte_hash/Makefile
> @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> # library name
> LIB = librte_hash.a
>
> -CFLAGS += -O3
> +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API
You need to update meson.build as well.
V1: To flush a ring not in use, dequeue one by one is wasting cpu cycles. The patch is to just resetting the head and tail indices to save cpu cycle. V2: Fix the coding style issue(commit message line too long) V3: Allow experimental API for meson build V4: - Include the ring perf test case enhancement patch in the series. - Replace ARRAY_SIZE with RTE_DIM. - Call memset to avoid clang compling complains. V5: - 1. Commit message tweaking for ring test case enhancement patch - 2. Upper to lower for mails to make match/grep more easily V6: Made upper case for the user name to comply with the convention. V7: Leave the ring test case patch out to other series as it is not closely linked to this series in logic Gavin Hu (2): ring: add reset API to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 5 files changed, 35 insertions(+), 8 deletions(-) -- 2.7.4
V1: To flush a ring not in use, dequeue one by one is wasting cpu cycles. The patch is to just resetting the head and tail indices to save cpu cycle. V2: Fix the coding style issue(commit message line too long) V3: Allow experimental API for meson build V4: - Include the ring perf test case enhancement patch in the series. - Replace ARRAY_SIZE with RTE_DIM. - Call memset to avoid clang compling complains. V5: - 1. Commit message tweaking for ring test case enhancement patch - 2. Upper to lower for mails to make match/grep more easily V6: Made upper case for the user name to comply with the convention. V7: Leave the ring test case patch out to other series as it is not closely linked to this series in logic Gavin Hu (2): ring: add reset API to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 5 files changed, 35 insertions(+), 8 deletions(-) -- 2.7.4
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> --- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index af5444a..2830300 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +static inline void __rte_experimental +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd..581d9ca 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.7.4
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> --- lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 2 files changed, 27 insertions(+) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index af5444a..2830300 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +static inline void __rte_experimental +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd..581d9ca 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.7.4
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435d..5669d83 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/meson.build b/lib/librte_hash/meson.build index efc06ed..ebf70de 100644 --- a/lib/librte_hash/meson.build +++ b/lib/librte_hash/meson.build @@ -14,3 +14,6 @@ headers = files('rte_cmp_arm64.h', sources = files('rte_cuckoo_hash.c', 'rte_fbk_hash.c') deps += ['ring'] + +# rte ring reset is not yet part of stable API +allow_experimental_apis = true diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index c01489b..4b08049 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -559,7 +559,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -570,16 +569,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.7.4
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435d..5669d83 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/meson.build b/lib/librte_hash/meson.build index efc06ed..ebf70de 100644 --- a/lib/librte_hash/meson.build +++ b/lib/librte_hash/meson.build @@ -14,3 +14,6 @@ headers = files('rte_cmp_arm64.h', sources = files('rte_cuckoo_hash.c', 'rte_fbk_hash.c') deps += ['ring'] + +# rte ring reset is not yet part of stable API +allow_experimental_apis = true diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index c01489b..4b08049 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -559,7 +559,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -570,16 +569,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.7.4
Hi,
On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> Currently, the flush is done by dequeuing the ring in a while loop. It is
> much simpler to flush the queue by resetting the head and tail indices.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++
> lib/librte_ring/rte_ring_version.map | 7 +++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> index af5444a..2830300 100644
> --- a/lib/librte_ring/rte_ring.h
> +++ b/lib/librte_ring/rte_ring.h
> @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p)
> }
>
> /**
> + * Flush a ring.
> + *
> + * This function flush all the elements in a ring
> + *
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * @warning
> + * Make sure the ring is not in use while calling this function.
> + *
> + * @param r
> + * A pointer to the ring structure.
> + */
> +static inline void __rte_experimental
> +rte_ring_reset(struct rte_ring *r)
> +{
> + r->prod.head = r->cons.head = 0;
> + r->prod.tail = r->cons.tail = 0;
> +}
> +
> +/**
> * Return the number of entries in a ring.
> *
> * @param r
> diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map
> index d935efd..581d9ca 100644
> --- a/lib/librte_ring/rte_ring_version.map
> +++ b/lib/librte_ring/rte_ring_version.map
> @@ -17,3 +17,10 @@ DPDK_2.2 {
> rte_ring_free;
>
> } DPDK_2.0;
> +
> +EXPERIMENTAL {
> + global:
> +
> + rte_ring_reset;
> +
> +};
> --
> 2.7.4
>
To me, a static inline function does not need to be added in
rte_ring_version.map (or is it due to a check script checking the
__rte_experimental tag ?). I found at least one commit where it
is not the case:
c277b34c1b3b ("mbuf: add function returning buffer address")
There are 2 options:
1- remove the rte_ring_version.map part of the patch.
2- change the static inline function into a standard function.
I would prefer 2-, because it allows to keep an api/abi compat
layer in the future.
Regards
Olivier
Hi,
On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> Currently, the flush is done by dequeuing the ring in a while loop. It is
> much simpler to flush the queue by resetting the head and tail indices.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> lib/librte_ring/rte_ring.h | 20 ++++++++++++++++++++
> lib/librte_ring/rte_ring_version.map | 7 +++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> index af5444a..2830300 100644
> --- a/lib/librte_ring/rte_ring.h
> +++ b/lib/librte_ring/rte_ring.h
> @@ -671,6 +671,26 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p)
> }
>
> /**
> + * Flush a ring.
> + *
> + * This function flush all the elements in a ring
> + *
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * @warning
> + * Make sure the ring is not in use while calling this function.
> + *
> + * @param r
> + * A pointer to the ring structure.
> + */
> +static inline void __rte_experimental
> +rte_ring_reset(struct rte_ring *r)
> +{
> + r->prod.head = r->cons.head = 0;
> + r->prod.tail = r->cons.tail = 0;
> +}
> +
> +/**
> * Return the number of entries in a ring.
> *
> * @param r
> diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map
> index d935efd..581d9ca 100644
> --- a/lib/librte_ring/rte_ring_version.map
> +++ b/lib/librte_ring/rte_ring_version.map
> @@ -17,3 +17,10 @@ DPDK_2.2 {
> rte_ring_free;
>
> } DPDK_2.0;
> +
> +EXPERIMENTAL {
> + global:
> +
> + rte_ring_reset;
> +
> +};
> --
> 2.7.4
>
To me, a static inline function does not need to be added in
rte_ring_version.map (or is it due to a check script checking the
__rte_experimental tag ?). I found at least one commit where it
is not the case:
c277b34c1b3b ("mbuf: add function returning buffer address")
There are 2 options:
1- remove the rte_ring_version.map part of the patch.
2- change the static inline function into a standard function.
I would prefer 2-, because it allows to keep an api/abi compat
layer in the future.
Regards
Olivier
29/03/2019 15:17, Olivier Matz:
> Hi,
>
> On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > much simpler to flush the queue by resetting the head and tail indices.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> > --- a/lib/librte_ring/rte_ring_version.map
> > +++ b/lib/librte_ring/rte_ring_version.map
> > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > rte_ring_free;
> >
> > } DPDK_2.0;
> > +
> > +EXPERIMENTAL {
> > + global:
> > +
> > + rte_ring_reset;
> > +
> > +};
>
> To me, a static inline function does not need to be added in
> rte_ring_version.map (or is it due to a check script checking the
> __rte_experimental tag ?). I found at least one commit where it
> is not the case:
> c277b34c1b3b ("mbuf: add function returning buffer address")
>
> There are 2 options:
> 1- remove the rte_ring_version.map part of the patch.
> 2- change the static inline function into a standard function.
>
> I would prefer 2-, because it allows to keep an api/abi compat
> layer in the future.
There are no news about this patch.
I classify it as changes requested.
V2: - fix the coding style issue(commit message line too long) V3: - allow experimental API for meson build V4: - include the ring perf test case enhancement patch in the series. - replace ARRAY_SIZE with RTE_DIM. - call memset to avoid clang compling complains. V5: - commit message tweaking for ring test case enhancement patch - upper to lower for mails to make match/grep more easily V6: - make upper case for the user name to comply with the convention. V7: - leave the ring test case patch out as not closely linked to this series in logic V8: - change the static inline function into a standard function to keep an api/abi compat layer in the future Gavin Hu (2): ring: add reset API to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.h | 21 +++++++++++++++++++++ 4 files changed, 29 insertions(+), 8 deletions(-) -- 2.7.4
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> --- lib/librte_ring/rte_ring.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index e5a175c..0213b08 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -669,6 +669,27 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +__rte_experimental +static inline void +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + +/** * Return the number of entries in a ring. * * @param r -- 2.7.4
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435d..5669d83 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/meson.build b/lib/librte_hash/meson.build index efc06ed..ebf70de 100644 --- a/lib/librte_hash/meson.build +++ b/lib/librte_hash/meson.build @@ -14,3 +14,6 @@ headers = files('rte_cmp_arm64.h', sources = files('rte_cuckoo_hash.c', 'rte_fbk_hash.c') deps += ['ring'] + +# rte ring reset is not yet part of stable API +allow_experimental_apis = true diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index d250938..87a4c01 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -570,7 +570,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -581,16 +580,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.7.4
Hi Olivier and Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, July 4, 2019 10:42 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; nd
> <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> when not in use
>
> 29/03/2019 15:17, Olivier Matz:
> > Hi,
> >
> > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > > much simpler to flush the queue by resetting the head and tail indices.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > ---
> > > --- a/lib/librte_ring/rte_ring_version.map
> > > +++ b/lib/librte_ring/rte_ring_version.map
> > > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > > rte_ring_free;
> > >
> > > } DPDK_2.0;
> > > +
> > > +EXPERIMENTAL {
> > > + global:
> > > +
> > > + rte_ring_reset;
> > > +
> > > +};
> >
> > To me, a static inline function does not need to be added in
> > rte_ring_version.map (or is it due to a check script checking the
> > __rte_experimental tag ?). I found at least one commit where it
> > is not the case:
> > c277b34c1b3b ("mbuf: add function returning buffer address")
> >
> > There are 2 options:
> > 1- remove the rte_ring_version.map part of the patch.
> > 2- change the static inline function into a standard function.
> >
> > I would prefer 2-, because it allows to keep an api/abi compat
> > layer in the future.
>
> There are no news about this patch.
> I classify it as changes requested.
>
Sorry for missed your comments for long time, I just submitted v8.
I took the first option as it is in the data path and to keep consistent to its neighboring functions.
Hi Gavin,
On Fri, Jul 12, 2019 at 09:32:39AM +0000, Gavin Hu (Arm Technology China) wrote:
> Hi Olivier and Thomas,
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Thursday, July 4, 2019 10:42 PM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; nd
> > <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> > Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com;
> > stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> > when not in use
> >
> > 29/03/2019 15:17, Olivier Matz:
> > > Hi,
> > >
> > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > > > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > > > much simpler to flush the queue by resetting the head and tail indices.
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > ---
> > > > --- a/lib/librte_ring/rte_ring_version.map
> > > > +++ b/lib/librte_ring/rte_ring_version.map
> > > > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > > > rte_ring_free;
> > > >
> > > > } DPDK_2.0;
> > > > +
> > > > +EXPERIMENTAL {
> > > > + global:
> > > > +
> > > > + rte_ring_reset;
> > > > +
> > > > +};
> > >
> > > To me, a static inline function does not need to be added in
> > > rte_ring_version.map (or is it due to a check script checking the
> > > __rte_experimental tag ?). I found at least one commit where it
> > > is not the case:
> > > c277b34c1b3b ("mbuf: add function returning buffer address")
> > >
> > > There are 2 options:
> > > 1- remove the rte_ring_version.map part of the patch.
> > > 2- change the static inline function into a standard function.
> > >
> > > I would prefer 2-, because it allows to keep an api/abi compat
> > > layer in the future.
> >
> > There are no news about this patch.
> > I classify it as changes requested.
> >
> Sorry for missed your comments for long time, I just submitted v8.
> I took the first option as it is in the data path and to keep consistent to its neighboring functions.
Could you give a little more context about why you need to reset
the ring in the data path? I see that it is used in rte_hash_reset(),
but in my thinking, this was more used at init/exit.
Thanks,
Olivier
Hi Olivier, > -----Original Message----- > From: Olivier Matz <olivier.matz@6wind.com> > Sent: Friday, July 12, 2019 5:54 PM > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com> > Cc: thomas@monjalon.net; dev@dpdk.org; nd <nd@arm.com>; > jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; > i.maximets@samsung.com; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring > when not in use > > Hi Gavin, > > On Fri, Jul 12, 2019 at 09:32:39AM +0000, Gavin Hu (Arm Technology China) > wrote: > > Hi Olivier and Thomas, > > > > > -----Original Message----- > > > From: Thomas Monjalon <thomas@monjalon.net> > > > Sent: Thursday, July 4, 2019 10:42 PM > > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com> > > > Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; nd > > > <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com; > > > Nipun.gupta@nxp.com; Honnappa Nagarahalli > > > <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com; > > > stable@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring > > > when not in use > > > > > > 29/03/2019 15:17, Olivier Matz: > > > > Hi, > > > > > > > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote: > > > > > Currently, the flush is done by dequeuing the ring in a while loop. It is > > > > > much simpler to flush the queue by resetting the head and tail indices. > > > > > > > > > > Fixes: af75078fece3 ("first public release") > > > > > Cc: stable@dpdk.org > > > > > > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com> > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > > > > > --- > > > > > --- a/lib/librte_ring/rte_ring_version.map > > > > > +++ b/lib/librte_ring/rte_ring_version.map > > > > > @@ -17,3 +17,10 @@ DPDK_2.2 { > > > > > rte_ring_free; > > > > > > > > > > } DPDK_2.0; > > > > > + > > > > > +EXPERIMENTAL { > > > > > + global: > > > > > + > > > > > + rte_ring_reset; > > > > > + > > > > > +}; > > > > > > > > To me, a static inline function does not need to be added in > > > > rte_ring_version.map (or is it due to a check script checking the > > > > __rte_experimental tag ?). I found at least one commit where it > > > > is not the case: > > > > c277b34c1b3b ("mbuf: add function returning buffer address") > > > > > > > > There are 2 options: > > > > 1- remove the rte_ring_version.map part of the patch. > > > > 2- change the static inline function into a standard function. > > > > > > > > I would prefer 2-, because it allows to keep an api/abi compat > > > > layer in the future. > > > > > > There are no news about this patch. > > > I classify it as changes requested. > > > > > Sorry for missed your comments for long time, I just submitted v8. > > I took the first option as it is in the data path and to keep consistent to its > neighboring functions. > > Could you give a little more context about why you need to reset > the ring in the data path? I see that it is used in rte_hash_reset(), > but in my thinking, this was more used at init/exit. Sorry,literally it is in the control path, but I was impressed it will impact the Data path performance when discussing this patch with Honnappa. > > Thanks, > Olivier
Hi Gavin,
On Fri, Jul 12, 2019 at 11:06:28AM +0000, Gavin Hu (Arm Technology China) wrote:
> Hi Olivier,
>
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Friday, July 12, 2019 5:54 PM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > Cc: thomas@monjalon.net; dev@dpdk.org; nd <nd@arm.com>;
> > jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > i.maximets@samsung.com; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> > when not in use
> >
> > Hi Gavin,
> >
> > On Fri, Jul 12, 2019 at 09:32:39AM +0000, Gavin Hu (Arm Technology China)
> > wrote:
> > > Hi Olivier and Thomas,
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Sent: Thursday, July 4, 2019 10:42 PM
> > > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > > > Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; nd
> > > > <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> > > > Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com;
> > > > stable@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> > > > when not in use
> > > >
> > > > 29/03/2019 15:17, Olivier Matz:
> > > > > Hi,
> > > > >
> > > > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > > > > > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > > > > > much simpler to flush the queue by resetting the head and tail indices.
> > > > > >
> > > > > > Fixes: af75078fece3 ("first public release")
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > > ---
> > > > > > --- a/lib/librte_ring/rte_ring_version.map
> > > > > > +++ b/lib/librte_ring/rte_ring_version.map
> > > > > > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > > > > > rte_ring_free;
> > > > > >
> > > > > > } DPDK_2.0;
> > > > > > +
> > > > > > +EXPERIMENTAL {
> > > > > > + global:
> > > > > > +
> > > > > > + rte_ring_reset;
> > > > > > +
> > > > > > +};
> > > > >
> > > > > To me, a static inline function does not need to be added in
> > > > > rte_ring_version.map (or is it due to a check script checking the
> > > > > __rte_experimental tag ?). I found at least one commit where it
> > > > > is not the case:
> > > > > c277b34c1b3b ("mbuf: add function returning buffer address")
> > > > >
> > > > > There are 2 options:
> > > > > 1- remove the rte_ring_version.map part of the patch.
> > > > > 2- change the static inline function into a standard function.
> > > > >
> > > > > I would prefer 2-, because it allows to keep an api/abi compat
> > > > > layer in the future.
> > > >
> > > > There are no news about this patch.
> > > > I classify it as changes requested.
> > > >
> > > Sorry for missed your comments for long time, I just submitted v8.
> > > I took the first option as it is in the data path and to keep consistent to its
> > neighboring functions.
> >
> > Could you give a little more context about why you need to reset
> > the ring in the data path? I see that it is used in rte_hash_reset(),
> > but in my thinking, this was more used at init/exit.
> Sorry,literally it is in the control path, but I was impressed it will impact the
> Data path performance when discussing this patch with Honnappa.
I'm asking this because given the recent discussions about ABI stability,
I'd like to avoid defining a new static inline if it is not required.
Thanks,
Olivier
Hi Olivier,
> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Friday, July 12, 2019 7:49 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Cc: thomas@monjalon.net; dev@dpdk.org; nd <nd@arm.com>;
> jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> i.maximets@samsung.com; stable@dpdk.org; Ruifeng Wang (Arm
> Technology China) <Ruifeng.Wang@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> when not in use
>
> Hi Gavin,
>
> On Fri, Jul 12, 2019 at 11:06:28AM +0000, Gavin Hu (Arm Technology China)
> wrote:
> > Hi Olivier,
> >
> > > -----Original Message-----
> > > From: Olivier Matz <olivier.matz@6wind.com>
> > > Sent: Friday, July 12, 2019 5:54 PM
> > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > > Cc: thomas@monjalon.net; dev@dpdk.org; nd <nd@arm.com>;
> > > jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com;
> > > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > > i.maximets@samsung.com; stable@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the
> ring
> > > when not in use
> > >
> > > Hi Gavin,
> > >
> > > On Fri, Jul 12, 2019 at 09:32:39AM +0000, Gavin Hu (Arm Technology
> China)
> > > wrote:
> > > > Hi Olivier and Thomas,
> > > >
> > > > > -----Original Message-----
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > Sent: Thursday, July 4, 2019 10:42 PM
> > > > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > > > > Cc: dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; nd
> > > > > <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> > > > > Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > > > > <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com;
> > > > > stable@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush
> the ring
> > > > > when not in use
> > > > >
> > > > > 29/03/2019 15:17, Olivier Matz:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > > > > > > Currently, the flush is done by dequeuing the ring in a while loop.
> It is
> > > > > > > much simpler to flush the queue by resetting the head and tail
> indices.
> > > > > > >
> > > > > > > Fixes: af75078fece3 ("first public release")
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > Reviewed-by: Honnappa Nagarahalli
> <honnappa.nagarahalli@arm.com>
> > > > > > > ---
> > > > > > > --- a/lib/librte_ring/rte_ring_version.map
> > > > > > > +++ b/lib/librte_ring/rte_ring_version.map
> > > > > > > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > > > > > > rte_ring_free;
> > > > > > >
> > > > > > > } DPDK_2.0;
> > > > > > > +
> > > > > > > +EXPERIMENTAL {
> > > > > > > + global:
> > > > > > > +
> > > > > > > + rte_ring_reset;
> > > > > > > +
> > > > > > > +};
> > > > > >
> > > > > > To me, a static inline function does not need to be added in
> > > > > > rte_ring_version.map (or is it due to a check script checking the
> > > > > > __rte_experimental tag ?). I found at least one commit where it
> > > > > > is not the case:
> > > > > > c277b34c1b3b ("mbuf: add function returning buffer address")
> > > > > >
> > > > > > There are 2 options:
> > > > > > 1- remove the rte_ring_version.map part of the patch.
> > > > > > 2- change the static inline function into a standard function.
> > > > > >
> > > > > > I would prefer 2-, because it allows to keep an api/abi compat
> > > > > > layer in the future.
> > > > >
> > > > > There are no news about this patch.
> > > > > I classify it as changes requested.
> > > > >
> > > > Sorry for missed your comments for long time, I just submitted v8.
> > > > I took the first option as it is in the data path and to keep consistent to
> its
> > > neighboring functions.
> > >
> > > Could you give a little more context about why you need to reset
> > > the ring in the data path? I see that it is used in rte_hash_reset(),
> > > but in my thinking, this was more used at init/exit.
> > Sorry,literally it is in the control path, but I was impressed it will impact
> the
> > Data path performance when discussing this patch with Honnappa.
>
> I'm asking this because given the recent discussions about ABI stability,
> I'd like to avoid defining a new static inline if it is not required.
Ok, will take 2nd option in V9, and squash the two patches into one, otherwise it reports the following error:
"error: ‘rte_ring_reset’ defined but not used [-Werror=unused-function]"
Best regards,
Gavin
<snip> > > > > > > > > > > > > 29/03/2019 15:17, Olivier Matz: > > > > > > > Hi, > > > > > > > > > > > > > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote: > > > > > > > > Currently, the flush is done by dequeuing the ring in a while loop. > > It is > > > > > > > > much simpler to flush the queue by resetting the head and > > > > > > > > tail > > indices. > > > > > > > > > > > > > > > > Fixes: af75078fece3 ("first public release") > > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com> > > > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > > > > > > > Reviewed-by: Honnappa Nagarahalli > > <honnappa.nagarahalli@arm.com> > > > > > > > > --- > > > > > > > > --- a/lib/librte_ring/rte_ring_version.map > > > > > > > > +++ b/lib/librte_ring/rte_ring_version.map > > > > > > > > @@ -17,3 +17,10 @@ DPDK_2.2 { > > > > > > > > rte_ring_free; > > > > > > > > > > > > > > > > } DPDK_2.0; > > > > > > > > + > > > > > > > > +EXPERIMENTAL { > > > > > > > > + global: > > > > > > > > + > > > > > > > > + rte_ring_reset; > > > > > > > > + > > > > > > > > +}; > > > > > > > > > > > > > > To me, a static inline function does not need to be added in > > > > > > > rte_ring_version.map (or is it due to a check script > > > > > > > checking the __rte_experimental tag ?). I found at least one > > > > > > > commit where it is not the case: > > > > > > > c277b34c1b3b ("mbuf: add function returning buffer address") > > > > > > > > > > > > > > There are 2 options: > > > > > > > 1- remove the rte_ring_version.map part of the patch. > > > > > > > 2- change the static inline function into a standard function. > > > > > > > > > > > > > > I would prefer 2-, because it allows to keep an api/abi > > > > > > > compat layer in the future. > > > > > > > > > > > > There are no news about this patch. > > > > > > I classify it as changes requested. > > > > > > > > > > > Sorry for missed your comments for long time, I just submitted v8. > > > > > I took the first option as it is in the data path and to keep > > > > > consistent to > > its > > > > neighboring functions. > > > > > > > > Could you give a little more context about why you need to reset > > > > the ring in the data path? I see that it is used in > > > > rte_hash_reset(), but in my thinking, this was more used at init/exit. > > > Sorry,literally it is in the control path, but I was impressed it > > > will impact > > the > > > Data path performance when discussing this patch with Honnappa. > > > > I'm asking this because given the recent discussions about ABI > > stability, I'd like to avoid defining a new static inline if it is not required. > > Ok, will take 2nd option in V9, and squash the two patches into one, otherwise > it reports the following error: > "error: ‘rte_ring_reset’ defined but not used [-Werror=unused-function]" Agree, it is a control path function, does not impact any data path performance. It should not be inline. > > Best regards, > Gavin
V2: - fix the coding style issue(commit message line too long) V3: - allow experimental API for meson build V4: - include the ring perf test case enhancement patch in the series. - replace ARRAY_SIZE with RTE_DIM. - call memset to avoid clang compling complains. V5: - commit message tweaking for ring test case enhancement patch - upper to lower for mails to make match/grep more easily V6: - make upper case for the user name to comply with the convention. V7: - leave the ring test case patch out as not closely linked to this series in logic V8: - the static inline function is removed from the rte_ring_version.map V9: - change the static inline function into a standard function to keep an api/abi compat layer in the future - add back the change to the rte_ring_version.map file Gavin Hu (2): ring: add reset API to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.c | 7 +++++++ lib/librte_ring/rte_ring.h | 17 +++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 6 files changed, 39 insertions(+), 8 deletions(-) -- 2.7.4
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> --- lib/librte_ring/rte_ring.c | 7 +++++++ lib/librte_ring/rte_ring.h | 17 +++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 3 files changed, 31 insertions(+) diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index b30b2aa..d9b3080 100644 --- a/lib/librte_ring/rte_ring.c +++ b/lib/librte_ring/rte_ring.c @@ -63,6 +63,13 @@ rte_ring_get_memsize(unsigned count) return sz; } +void +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + int rte_ring_init(struct rte_ring *r, const char *name, unsigned count, unsigned flags) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index e5a175c..2a9f768 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -669,6 +669,23 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +__rte_experimental +void +rte_ring_reset(struct rte_ring *r); + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd..581d9ca 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.7.4
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Fixes: b26473ff8f4a ("hash: add reset function") Fixes: 75706568a7eb ("hash: add extendable bucket feature") Cc: stable@dpdk.org Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435d..5669d83 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/meson.build b/lib/librte_hash/meson.build index efc06ed..ebf70de 100644 --- a/lib/librte_hash/meson.build +++ b/lib/librte_hash/meson.build @@ -14,3 +14,6 @@ headers = files('rte_cmp_arm64.h', sources = files('rte_cuckoo_hash.c', 'rte_fbk_hash.c') deps += ['ring'] + +# rte ring reset is not yet part of stable API +allow_experimental_apis = true diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index d250938..87a4c01 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -570,7 +570,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -581,16 +580,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.7.4
> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Friday, July 12, 2019 11:41 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Olivier Matz
> <olivier.matz@6wind.com>
> Cc: thomas@monjalon.net; dev@dpdk.org; nd <nd@arm.com>;
> jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com;
> i.maximets@samsung.com; stable@dpdk.org; Ruifeng Wang (Arm
> Technology China) <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring
> when not in use
>
> <snip>
>
> > > > > > >
> > > > > > > 29/03/2019 15:17, Olivier Matz:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > > > > > > > > Currently, the flush is done by dequeuing the ring in a while
> loop.
> > > It is
> > > > > > > > > much simpler to flush the queue by resetting the head and
> > > > > > > > > tail
> > > indices.
> > > > > > > > >
> > > > > > > > > Fixes: af75078fece3 ("first public release")
> > > > > > > > > Cc: stable@dpdk.org
> > > > > > > > >
> > > > > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > > > Reviewed-by: Honnappa Nagarahalli
> > > <honnappa.nagarahalli@arm.com>
> > > > > > > > > ---
> > > > > > > > > --- a/lib/librte_ring/rte_ring_version.map
> > > > > > > > > +++ b/lib/librte_ring/rte_ring_version.map
> > > > > > > > > @@ -17,3 +17,10 @@ DPDK_2.2 {
> > > > > > > > > rte_ring_free;
> > > > > > > > >
> > > > > > > > > } DPDK_2.0;
> > > > > > > > > +
> > > > > > > > > +EXPERIMENTAL {
> > > > > > > > > + global:
> > > > > > > > > +
> > > > > > > > > + rte_ring_reset;
> > > > > > > > > +
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > To me, a static inline function does not need to be added in
> > > > > > > > rte_ring_version.map (or is it due to a check script
> > > > > > > > checking the __rte_experimental tag ?). I found at least one
> > > > > > > > commit where it is not the case:
> > > > > > > > c277b34c1b3b ("mbuf: add function returning buffer address")
> > > > > > > >
> > > > > > > > There are 2 options:
> > > > > > > > 1- remove the rte_ring_version.map part of the patch.
> > > > > > > > 2- change the static inline function into a standard function.
> > > > > > > >
> > > > > > > > I would prefer 2-, because it allows to keep an api/abi
> > > > > > > > compat layer in the future.
> > > > > > >
> > > > > > > There are no news about this patch.
> > > > > > > I classify it as changes requested.
> > > > > > >
> > > > > > Sorry for missed your comments for long time, I just submitted v8.
> > > > > > I took the first option as it is in the data path and to keep
> > > > > > consistent to
> > > its
> > > > > neighboring functions.
> > > > >
> > > > > Could you give a little more context about why you need to reset
> > > > > the ring in the data path? I see that it is used in
> > > > > rte_hash_reset(), but in my thinking, this was more used at init/exit.
> > > > Sorry,literally it is in the control path, but I was impressed it
> > > > will impact
> > > the
> > > > Data path performance when discussing this patch with Honnappa.
> > >
> > > I'm asking this because given the recent discussions about ABI
> > > stability, I'd like to avoid defining a new static inline if it is not required.
> >
> > Ok, will take 2nd option in V9, and squash the two patches into one,
> otherwise
> > it reports the following error:
> > "error: ‘rte_ring_reset’ defined but not used [-Werror=unused-function]"
> Agree, it is a control path function, does not impact any data path
> performance. It should not be inline.
Thanks for your clarification, I submitted v9 for this.
On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> Currently, the flush is done by dequeuing the ring in a while loop. It is
> much simpler to flush the queue by resetting the head and tail indices.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
On Tue, Jul 16, 2019 at 10:47:20AM +0200, Olivier Matz wrote:
> On Fri, Mar 15, 2019 at 11:31:25AM +0800, Gavin Hu wrote:
> > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > much simpler to flush the queue by resetting the head and tail indices.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Sorry, I meant to ack the v9.
On Fri, Jul 12, 2019 at 11:54:36PM +0800, Gavin Hu wrote:
> Currently, the flush is done by dequeuing the ring in a while loop. It is
> much simpler to flush the queue by resetting the head and tail indices.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
On Tue, Jul 16, 2019 at 11:01:21AM +0200, Olivier Matz wrote: > On Fri, Jul 12, 2019 at 11:54:36PM +0800, Gavin Hu wrote: > > Currently, the flush is done by dequeuing the ring in a while loop. It is > > much simpler to flush the queue by resetting the head and tail indices. > > > > Fixes: af75078fece3 ("first public release") > > Cc: stable@dpdk.org Actually it's not a fix, it adds a new API. Is the patch in hash library intended to be backported? If yes, as it seems to be a performance optimization, you'll need to describe what scenario you're fixing and what is the performance gain. If no, the Cc stable can be removed. Thanks > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > > Acked-by: Olivier Matz <olivier.matz@6wind.com>
Hi Olivier, Thomas, > -----Original Message----- > From: Olivier Matz <olivier.matz@6wind.com> > Sent: Tuesday, July 16, 2019 6:32 AM > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com> > Cc: dev@dpdk.org; nd <nd@arm.com>; thomas@monjalon.net; > jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; > i.maximets@samsung.com; stable@dpdk.org > Subject: Re: [PATCH v9 1/2] ring: add reset API to flush the ring when not in > use > > On Tue, Jul 16, 2019 at 11:01:21AM +0200, Olivier Matz wrote: > > On Fri, Jul 12, 2019 at 11:54:36PM +0800, Gavin Hu wrote: > > > Currently, the flush is done by dequeuing the ring in a while loop. It is > > > much simpler to flush the queue by resetting the head and tail indices. > > > > > > Fixes: af75078fece3 ("first public release") > > > Cc: stable@dpdk.org > > Actually it's not a fix, it adds a new API. > > Is the patch in hash library intended to be backported? If yes, as it > seems to be a performance optimization, you'll need to describe what > scenario you're fixing and what is the performance gain. If no, the Cc > stable can be removed. As this is not in the data plan, I don't intend to backport. Do I need to submit a new version to remove the CC: lines? > > > > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > > > > Acked-by: Olivier Matz <olivier.matz@6wind.com>
16/07/2019 16:03, Gavin Hu (Arm Technology China):
> From: Olivier Matz <olivier.matz@6wind.com>
> > On Tue, Jul 16, 2019 at 11:01:21AM +0200, Olivier Matz wrote:
> > > On Fri, Jul 12, 2019 at 11:54:36PM +0800, Gavin Hu wrote:
> > > > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > > > much simpler to flush the queue by resetting the head and tail indices.
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Cc: stable@dpdk.org
> >
> > Actually it's not a fix, it adds a new API.
> >
> > Is the patch in hash library intended to be backported? If yes, as it
> > seems to be a performance optimization, you'll need to describe what
> > scenario you're fixing and what is the performance gain. If no, the Cc
> > stable can be removed.
>
> As this is not in the data plan, I don't intend to backport.
> Do I need to submit a new version to remove the CC: lines?
Yes please.
You can also remove the "fixes" line in the first patch.
V2: - fix the coding style issue(commit message line too long) V3: - allow experimental API for meson build V4: - include the ring perf test case enhancement patch in the series. - replace ARRAY_SIZE with RTE_DIM. - call memset to avoid clang compling complains. V5: - commit message tweaking for ring test case enhancement patch - upper to lower for mails to make match/grep more easily V6: - make upper case for the user name to comply with the convention. V7: - leave the ring test case patch out as not closely linked to this series in logic V8: - the static inline function is removed from the rte_ring_version.map V9: - change the static inline function into a standard function to keep an api/abi compat layer in the future - add back the change to the rte_ring_version.map file V10: - remove the CC: lines as not in the data plane. Gavin Hu (2): ring: add reset API to flush the ring when not in use hash: flush the rings instead of dequeuing one by one lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- lib/librte_ring/rte_ring.c | 7 +++++++ lib/librte_ring/rte_ring.h | 17 +++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 6 files changed, 39 insertions(+), 8 deletions(-) -- 2.7.4
Currently, the flush is done by dequeuing the ring in a while loop. It is much simpler to flush the queue by resetting the head and tail indices. Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Olivier Matz <olivier.matz@6wind.com> --- lib/librte_ring/rte_ring.c | 7 +++++++ lib/librte_ring/rte_ring.h | 17 +++++++++++++++++ lib/librte_ring/rte_ring_version.map | 7 +++++++ 3 files changed, 31 insertions(+) diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index b30b2aa..d9b3080 100644 --- a/lib/librte_ring/rte_ring.c +++ b/lib/librte_ring/rte_ring.c @@ -63,6 +63,13 @@ rte_ring_get_memsize(unsigned count) return sz; } +void +rte_ring_reset(struct rte_ring *r) +{ + r->prod.head = r->cons.head = 0; + r->prod.tail = r->cons.tail = 0; +} + int rte_ring_init(struct rte_ring *r, const char *name, unsigned count, unsigned flags) diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h index e5a175c..2a9f768 100644 --- a/lib/librte_ring/rte_ring.h +++ b/lib/librte_ring/rte_ring.h @@ -669,6 +669,23 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) } /** + * Flush a ring. + * + * This function flush all the elements in a ring + * + * @b EXPERIMENTAL: this API may change without prior notice + * + * @warning + * Make sure the ring is not in use while calling this function. + * + * @param r + * A pointer to the ring structure. + */ +__rte_experimental +void +rte_ring_reset(struct rte_ring *r); + +/** * Return the number of entries in a ring. * * @param r diff --git a/lib/librte_ring/rte_ring_version.map b/lib/librte_ring/rte_ring_version.map index d935efd..510c138 100644 --- a/lib/librte_ring/rte_ring_version.map +++ b/lib/librte_ring/rte_ring_version.map @@ -17,3 +17,10 @@ DPDK_2.2 { rte_ring_free; } DPDK_2.0; + +EXPERIMENTAL { + global: + + rte_ring_reset; + +}; -- 2.7.4
Within rte_hash_reset, calling a while loop to dequeue one by one from the ring, while not using them at all, is wasting cycles, The patch just flush the ring by resetting the indices can save cpu cycles. Signed-off-by: Gavin Hu <gavin.hu@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Acked-by: Yipeng Wang <yipeng1.wang@intel.com> --- lib/librte_hash/Makefile | 2 +- lib/librte_hash/meson.build | 3 +++ lib/librte_hash/rte_cuckoo_hash.c | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/librte_hash/Makefile b/lib/librte_hash/Makefile index c8c435d..5669d83 100644 --- a/lib/librte_hash/Makefile +++ b/lib/librte_hash/Makefile @@ -6,7 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk # library name LIB = librte_hash.a -CFLAGS += -O3 +CFLAGS += -O3 -DALLOW_EXPERIMENTAL_API CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) LDLIBS += -lrte_eal -lrte_ring diff --git a/lib/librte_hash/meson.build b/lib/librte_hash/meson.build index efc06ed..ebf70de 100644 --- a/lib/librte_hash/meson.build +++ b/lib/librte_hash/meson.build @@ -14,3 +14,6 @@ headers = files('rte_cmp_arm64.h', sources = files('rte_cuckoo_hash.c', 'rte_fbk_hash.c') deps += ['ring'] + +# rte ring reset is not yet part of stable API +allow_experimental_apis = true diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index d250938..87a4c01 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -570,7 +570,6 @@ __hash_rw_reader_unlock(const struct rte_hash *h) void rte_hash_reset(struct rte_hash *h) { - void *ptr; uint32_t tot_ring_cnt, i; if (h == NULL) @@ -581,16 +580,14 @@ rte_hash_reset(struct rte_hash *h) memset(h->key_store, 0, h->key_entry_size * (h->entries + 1)); *h->tbl_chng_cnt = 0; - /* clear the free ring */ - while (rte_ring_dequeue(h->free_slots, &ptr) == 0) - continue; + /* reset the free ring */ + rte_ring_reset(h->free_slots); - /* clear free extendable bucket ring and memory */ + /* flush free extendable bucket ring and memory */ if (h->ext_table_support) { memset(h->buckets_ext, 0, h->num_buckets * sizeof(struct rte_hash_bucket)); - while (rte_ring_dequeue(h->free_ext_bkts, &ptr) == 0) - continue; + rte_ring_reset(h->free_ext_bkts); } /* Repopulate the free slots ring. Entry zero is reserved for key misses */ -- 2.7.4
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, July 16, 2019 10:07 AM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; nd
> <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; i.maximets@samsung.com;
> stable@dpdk.org
> Subject: Re: [PATCH v9 1/2] ring: add reset API to flush the ring when not in
> use
>
> 16/07/2019 16:03, Gavin Hu (Arm Technology China):
> > From: Olivier Matz <olivier.matz@6wind.com>
> > > On Tue, Jul 16, 2019 at 11:01:21AM +0200, Olivier Matz wrote:
> > > > On Fri, Jul 12, 2019 at 11:54:36PM +0800, Gavin Hu wrote:
> > > > > Currently, the flush is done by dequeuing the ring in a while loop. It is
> > > > > much simpler to flush the queue by resetting the head and tail
> indices.
> > > > >
> > > > > Fixes: af75078fece3 ("first public release")
> > > > > Cc: stable@dpdk.org
> > >
> > > Actually it's not a fix, it adds a new API.
> > >
> > > Is the patch in hash library intended to be backported? If yes, as it
> > > seems to be a performance optimization, you'll need to describe what
> > > scenario you're fixing and what is the performance gain. If no, the Cc
> > > stable can be removed.
> >
> > As this is not in the data plan, I don't intend to backport.
> > Do I need to submit a new version to remove the CC: lines?
>
> Yes please.
> You can also remove the "fixes" line in the first patch.
Sure, just sent out V10, thanks!
> Gavin Hu (2):
> ring: add reset API to flush the ring when not in use
> hash: flush the rings instead of dequeuing one by one
Applied, thanks