patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
@ 2018-12-12  6:24   ` Gavin Hu
       [not found]   ` <20181212064733.1008-1-gavin.hu@arm.com>
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
       [not found]   ` <20181212064733.1008-1-gavin.hu@arm.com>
@ 2018-12-12  6:47     ` Gavin Hu
  2018-12-12 10:15       ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
  2018-12-12 19:28       ` Mattias Rönnblom
  0 siblings, 2 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:47     ` [dpdk-stable] [PATCH v2 " Gavin Hu
@ 2018-12-12 10:15       ` Bruce Richardson
  2018-12-12 19:28       ` Mattias Rönnblom
  1 sibling, 0 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one
  2018-12-12  6:47     ` [dpdk-stable] [PATCH v2 " Gavin Hu
  2018-12-12 10:15       ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
@ 2018-12-12 19:28       ` Mattias Rönnblom
  1 sibling, 0 replies; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
  2018-12-12  6:24   ` [dpdk-stable] [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
       [not found]   ` <20181212064733.1008-1-gavin.hu@arm.com>
@ 2019-03-15  3:31   ` Gavin Hu
  2019-03-29 14:17     ` Olivier Matz
  2019-07-16  8:47     ` [dpdk-stable] " Olivier Matz
  2019-03-15  3:31   ` [dpdk-stable] [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                     ` (4 subsequent siblings)
  7 siblings, 2 replies; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
                     ` (2 preceding siblings ...)
  2019-03-15  3:31   ` [dpdk-stable] [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-stable] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
                     ` (3 subsequent siblings)
  7 siblings, 0 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-03-15  3:31   ` [dpdk-stable] [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-stable] [dpdk-dev] " Thomas Monjalon
  2019-07-16  8:47     ` [dpdk-stable] " Olivier Matz
  1 sibling, 1 reply; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
                     ` (3 preceding siblings ...)
  2019-03-15  3:31   ` [dpdk-stable] [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-stable] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
                     ` (4 preceding siblings ...)
  2019-07-12  9:26   ` [dpdk-stable] [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-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
  2019-07-12 15:54   ` [dpdk-stable] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  7 siblings, 0 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-04 14:42       ` [dpdk-stable] [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; 26+ 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@nxp.com, 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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,literally it is in the control path, but I was impressed it will impact the 
Data path performance when discussing this patch with Honnappa. 
> 
> Thanks,
> Olivier

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

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
                     ` (5 preceding siblings ...)
  2019-07-12  9:26   ` [dpdk-stable] [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-16  9:01     ` Olivier Matz
  2019-07-12 15:54   ` [dpdk-stable] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
  7 siblings, 1 reply; 26+ 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] 26+ messages in thread

* [dpdk-stable] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one
       [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
                     ` (6 preceding siblings ...)
  2019-07-12 15:54   ` [dpdk-stable] [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
  7 siblings, 0 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-03-15  3:31   ` [dpdk-stable] [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; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use
  2019-07-16  8:47     ` [dpdk-stable] " Olivier Matz
@ 2019-07-16  9:00       ` Olivier Matz
  0 siblings, 0 replies; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use
  2019-07-12 15:54   ` [dpdk-stable] [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; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com,
	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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com, 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] 26+ messages in thread

* Re: [dpdk-stable] [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; 26+ 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@nxp.com, 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] 26+ messages in thread

end of thread, other threads:[~2019-07-16 19:25 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1552620686-10347-1-git-send-email-gavin.hu@arm.com>
     [not found] ` <20181212062404.30243-1-gavin.hu@arm.com>
2018-12-12  6:24   ` [dpdk-stable] [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
     [not found]   ` <20181212064733.1008-1-gavin.hu@arm.com>
2018-12-12  6:47     ` [dpdk-stable] [PATCH v2 " Gavin Hu
2018-12-12 10:15       ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2018-12-12 19:28       ` Mattias Rönnblom
2019-03-15  3:31   ` [dpdk-stable] [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-stable] [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     ` [dpdk-stable] " Olivier Matz
2019-07-16  9:00       ` Olivier Matz
2019-03-15  3:31   ` [dpdk-stable] [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2019-07-12  9:26   ` [dpdk-stable] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use Gavin Hu
2019-07-12  9:26   ` [dpdk-stable] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu
2019-07-12 15:54   ` [dpdk-stable] [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-stable] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one Gavin Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).