DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring
@ 2018-12-12  6:24 Gavin Hu
  2018-12-12  6:24 ` [PATCH v1 1/2] ring: add rte_ring_reset api to flush the ring Gavin Hu
                   ` (14 more replies)
  0 siblings, 15 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:24 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal, Honnappa.Nagarahalli

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v1 1/2] ring: add rte_ring_reset api to flush the ring
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
@ 2018-12-12  6:24 ` Gavin Hu
  2018-12-12  6:24 ` [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:24 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal, Honnappa.Nagarahalli

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
  2018-12-12  6:24 ` [PATCH v1 1/2] ring: add rte_ring_reset api to flush the ring Gavin Hu
@ 2018-12-12  6:24 ` Gavin Hu
  2018-12-12  6:47 ` [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash Gavin Hu
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:24 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal,
	Honnappa.Nagarahalli, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
  2018-12-12  6:24 ` [PATCH v1 1/2] ring: add rte_ring_reset api to flush the ring Gavin Hu
  2018-12-12  6:24 ` [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
@ 2018-12-12  6:47 ` Gavin Hu
  2018-12-12  6:47   ` [PATCH v2 1/2] ring: add reset api to flush the ring when not in use Gavin Hu
  2018-12-12  6:47   ` [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  2019-03-15  3:31 ` [PATCH v7 0/2] new ring reset api and use it by hash Gavin Hu
                   ` (11 subsequent siblings)
  14 siblings, 2 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:47 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal, Honnappa.Nagarahalli

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v2 1/2] ring: add reset api to flush the ring when not in use
  2018-12-12  6:47 ` [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash Gavin Hu
@ 2018-12-12  6:47   ` Gavin Hu
  2018-12-12  6:47   ` [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  1 sibling, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:47 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal, Honnappa.Nagarahalli

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:47 ` [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash Gavin Hu
  2018-12-12  6:47   ` [PATCH v2 1/2] ring: add reset api to flush the ring when not in use Gavin Hu
@ 2018-12-12  6:47   ` Gavin Hu
  2018-12-12 10:15     ` Bruce Richardson
  2018-12-12 19:28     ` Mattias Rönnblom
  1 sibling, 2 replies; 37+ messages in thread
From: Gavin Hu @ 2018-12-12  6:47 UTC (permalink / raw)
  To: dev
  Cc: nd, Gavin Hu, thomas, jerin.jacob, hemant.agrawal,
	Honnappa.Nagarahalli, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:47   ` [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
@ 2018-12-12 10:15     ` Bruce Richardson
  2018-12-12 19:28     ` Mattias Rönnblom
  1 sibling, 0 replies; 37+ messages in thread
From: Bruce Richardson @ 2018-12-12 10:15 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerin.jacob, hemant.agrawal,
	Honnappa.Nagarahalli, stable

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?

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:47   ` [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  2018-12-12 10:15     ` Bruce Richardson
@ 2018-12-12 19:28     ` Mattias Rönnblom
  1 sibling, 0 replies; 37+ messages in thread
From: Mattias Rönnblom @ 2018-12-12 19:28 UTC (permalink / raw)
  To: Gavin Hu, dev
  Cc: nd, thomas, jerin.jacob, hemant.agrawal, Honnappa.Nagarahalli, stable

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 0/2] new ring reset api and use it by hash
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (2 preceding siblings ...)
  2018-12-12  6:47 ` [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash Gavin Hu
@ 2019-03-15  3:31 ` Gavin Hu
  2019-03-15  3:31 ` [PATCH v7 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-03-15  3:31 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (3 preceding siblings ...)
  2019-03-15  3:31 ` [PATCH v7 0/2] new ring reset api and use it by hash Gavin Hu
@ 2019-03-15  3:31 ` Gavin Hu
  2019-03-29 14:17   ` Olivier Matz
  2019-07-16  8:47   ` Olivier Matz
  2019-03-15  3:31 ` [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                   ` (9 subsequent siblings)
  14 siblings, 2 replies; 37+ messages in thread
From: Gavin Hu @ 2019-03-15  3:31 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (4 preceding siblings ...)
  2019-03-15  3:31 ` [PATCH v7 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-03-15  3:31 ` Gavin Hu
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 0/2] new ring reset api and use it by hash Gavin Hu
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-03-15  3:31 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-03-15  3:31 ` [PATCH v7 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-03-29 14:17   ` Olivier Matz
  2019-07-04 14:42     ` [dpdk-dev] " Thomas Monjalon
  2019-07-16  8:47   ` Olivier Matz
  1 sibling, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-03-29 14:17 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-03-29 14:17   ` Olivier Matz
@ 2019-07-04 14:42     ` " Thomas Monjalon
  2019-07-12  9:32       ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2019-07-04 14:42 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, Olivier Matz, nd, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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.




^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v8 0/2] new ring reset api and use it by hash
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (5 preceding siblings ...)
  2019-03-15  3:31 ` [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
@ 2019-07-12  9:26 ` Gavin Hu
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-12  9:26 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (6 preceding siblings ...)
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 0/2] new ring reset api and use it by hash Gavin Hu
@ 2019-07-12  9:26 ` Gavin Hu
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-12  9:26 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (7 preceding siblings ...)
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-07-12  9:26 ` Gavin Hu
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 0/2] new ring reset api and use it by hash Gavin Hu
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-12  9:26 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-04 14:42     ` [dpdk-dev] " Thomas Monjalon
@ 2019-07-12  9:32       ` Gavin Hu (Arm Technology China)
  2019-07-12  9:53         ` Olivier Matz
  0 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-12  9:32 UTC (permalink / raw)
  To: thomas
  Cc: dev, Olivier Matz, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable

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. 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12  9:32       ` Gavin Hu (Arm Technology China)
@ 2019-07-12  9:53         ` Olivier Matz
  2019-07-12 11:06           ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-07-12  9:53 UTC (permalink / raw)
  To: Gavin Hu (Arm Technology China)
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12  9:53         ` Olivier Matz
@ 2019-07-12 11:06           ` Gavin Hu (Arm Technology China)
  2019-07-12 11:48             ` Olivier Matz
  0 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-12 11:06 UTC (permalink / raw)
  To: Olivier Matz
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable,
	Ruifeng Wang (Arm Technology China)

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^[$B!$^[(Bliterally 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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 11:06           ` Gavin Hu (Arm Technology China)
@ 2019-07-12 11:48             ` Olivier Matz
  2019-07-12 15:07               ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-07-12 11:48 UTC (permalink / raw)
  To: Gavin Hu (Arm Technology China)
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable,
	Ruifeng Wang (Arm Technology China)

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 11:48             ` Olivier Matz
@ 2019-07-12 15:07               ` Gavin Hu (Arm Technology China)
  2019-07-12 15:40                 ` Honnappa Nagarahalli
  0 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-12 15:07 UTC (permalink / raw)
  To: Olivier Matz
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable,
	Ruifeng Wang (Arm Technology China)

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 15:07               ` Gavin Hu (Arm Technology China)
@ 2019-07-12 15:40                 ` Honnappa Nagarahalli
  2019-07-12 16:01                   ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Honnappa Nagarahalli @ 2019-07-12 15:40 UTC (permalink / raw)
  To: Gavin Hu (Arm Technology China), Olivier Matz
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta, i.maximets,
	stable, Ruifeng Wang (Arm Technology China),
	nd

<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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v9 0/2] new ring reset api and use it by hash
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (8 preceding siblings ...)
  2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
@ 2019-07-12 15:54 ` Gavin Hu
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-12 15:54 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (9 preceding siblings ...)
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 0/2] new ring reset api and use it by hash Gavin Hu
@ 2019-07-12 15:54 ` Gavin Hu
  2019-07-16  9:01   ` Olivier Matz
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                   ` (3 subsequent siblings)
  14 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu @ 2019-07-12 15:54 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (10 preceding siblings ...)
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-07-12 15:54 ` Gavin Hu
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash Gavin Hu
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-12 15:54 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, i.maximets, olivier.matz, stable

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 15:40                 ` Honnappa Nagarahalli
@ 2019-07-12 16:01                   ` Gavin Hu (Arm Technology China)
  0 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-12 16:01 UTC (permalink / raw)
  To: Honnappa Nagarahalli, Olivier Matz
  Cc: thomas, dev, nd, jerinj, hemant.agrawal, Nipun.gupta, i.maximets,
	stable, Ruifeng Wang (Arm Technology China),
	nd

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-03-15  3:31 ` [PATCH v7 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
  2019-03-29 14:17   ` Olivier Matz
@ 2019-07-16  8:47   ` Olivier Matz
  2019-07-16  9:00     ` Olivier Matz
  1 sibling, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-07-16  8:47 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16  8:47   ` Olivier Matz
@ 2019-07-16  9:00     ` Olivier Matz
  0 siblings, 0 replies; 37+ messages in thread
From: Olivier Matz @ 2019-07-16  9:00 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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.


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-07-16  9:01   ` Olivier Matz
  2019-07-16 11:31     ` Olivier Matz
  0 siblings, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-07-16  9:01 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16  9:01   ` Olivier Matz
@ 2019-07-16 11:31     ` Olivier Matz
  2019-07-16 14:03       ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Olivier Matz @ 2019-07-16 11:31 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, i.maximets, stable

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>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16 11:31     ` Olivier Matz
@ 2019-07-16 14:03       ` Gavin Hu (Arm Technology China)
  2019-07-16 15:06         ` Thomas Monjalon
  0 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-16 14:03 UTC (permalink / raw)
  To: Olivier Matz, thomas
  Cc: dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable,
	Gavin Hu (Arm Technology China)

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>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16 14:03       ` Gavin Hu (Arm Technology China)
@ 2019-07-16 15:06         ` Thomas Monjalon
  2019-07-16 19:25           ` Gavin Hu (Arm Technology China)
  0 siblings, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2019-07-16 15:06 UTC (permalink / raw)
  To: Gavin Hu (Arm Technology China)
  Cc: Olivier Matz, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable

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.



^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (11 preceding siblings ...)
  2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
@ 2019-07-16 19:23 ` Gavin Hu
  2019-07-17 17:53   ` Thomas Monjalon
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  14 siblings, 1 reply; 37+ messages in thread
From: Gavin Hu @ 2019-07-16 19:23 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, olivier.matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v10 1/2] ring: add reset API to flush the ring when not in use
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (12 preceding siblings ...)
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash Gavin Hu
@ 2019-07-16 19:23 ` Gavin Hu
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-16 19:23 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, olivier.matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [dpdk-dev] [PATCH v10 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
                   ` (13 preceding siblings ...)
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
@ 2019-07-16 19:23 ` Gavin Hu
  14 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu @ 2019-07-16 19:23 UTC (permalink / raw)
  To: dev
  Cc: nd, thomas, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, gavin.hu, olivier.matz

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16 15:06         ` Thomas Monjalon
@ 2019-07-16 19:25           ` Gavin Hu (Arm Technology China)
  0 siblings, 0 replies; 37+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-07-16 19:25 UTC (permalink / raw)
  To: thomas
  Cc: Olivier Matz, dev, nd, jerinj, hemant.agrawal, Nipun.gupta,
	Honnappa Nagarahalli, i.maximets, stable

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!


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash
  2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash Gavin Hu
@ 2019-07-17 17:53   ` Thomas Monjalon
  0 siblings, 0 replies; 37+ messages in thread
From: Thomas Monjalon @ 2019-07-17 17:53 UTC (permalink / raw)
  To: Gavin Hu
  Cc: dev, nd, jerinj, hemant.agrawal, nipun.gupta,
	Honnappa.Nagarahalli, olivier.matz

> 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




^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, back to index

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12  6:24 [PATCH v1 0/2] add rte_ring_reset and use it to flush a ring Gavin Hu
2018-12-12  6:24 ` [PATCH v1 1/2] ring: add rte_ring_reset api to flush the ring Gavin Hu
2018-12-12  6:24 ` [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2018-12-12  6:47 ` [PATCH v2 0/2] add rte ring reset api and use it to flush a ring by hash Gavin Hu
2018-12-12  6:47   ` [PATCH v2 1/2] ring: add reset api to flush the ring when not in use Gavin Hu
2018-12-12  6:47   ` [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2018-12-12 10:15     ` Bruce Richardson
2018-12-12 19:28     ` Mattias Rönnblom
2019-03-15  3:31 ` [PATCH v7 0/2] new ring reset api and use it by hash Gavin Hu
2019-03-15  3:31 ` [PATCH v7 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
2019-03-29 14:17   ` Olivier Matz
2019-07-04 14:42     ` [dpdk-dev] " Thomas Monjalon
2019-07-12  9:32       ` Gavin Hu (Arm Technology China)
2019-07-12  9:53         ` Olivier Matz
2019-07-12 11:06           ` Gavin Hu (Arm Technology China)
2019-07-12 11:48             ` Olivier Matz
2019-07-12 15:07               ` Gavin Hu (Arm Technology China)
2019-07-12 15:40                 ` Honnappa Nagarahalli
2019-07-12 16:01                   ` Gavin Hu (Arm Technology China)
2019-07-16  8:47   ` Olivier Matz
2019-07-16  9:00     ` Olivier Matz
2019-03-15  3:31 ` [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 0/2] new ring reset api and use it by hash Gavin Hu
2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
2019-07-12  9:26 ` [dpdk-dev] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 0/2] new ring reset api and use it by hash Gavin Hu
2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
2019-07-16  9:01   ` Olivier Matz
2019-07-16 11:31     ` Olivier Matz
2019-07-16 14:03       ` Gavin Hu (Arm Technology China)
2019-07-16 15:06         ` Thomas Monjalon
2019-07-16 19:25           ` Gavin Hu (Arm Technology China)
2019-07-12 15:54 ` [dpdk-dev] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 0/2] new ring reset api and use it by hash Gavin Hu
2019-07-17 17:53   ` Thomas Monjalon
2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
2019-07-16 19:23 ` [dpdk-dev] [PATCH v10 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox