[dpdk-stable] [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one

[dpdk-stable] [PATCH v1 2/2] hash: flush the rings instead of dequeuing one by one

[Gavin Hu]

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

[dpdk-stable] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one

[Gavin Hu]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one

[Bruce Richardson]

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?

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] hash: flush the rings instead of dequeuing one by one

[Mattias Rönnblom]

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.

[dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu]

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

Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Thomas Monjalon]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu (Arm Technology China)]

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. 

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu (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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu (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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Honnappa Nagarahalli]

<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

Re: [dpdk-stable] [dpdk-dev] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu (Arm Technology China)]

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

Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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>

Re: [dpdk-stable] [PATCH v7 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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.

[dpdk-stable] [PATCH v7 2/2] hash: flush the rings instead of dequeuing one by one

[Gavin Hu]

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

[dpdk-stable] [PATCH v8 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu]

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

[dpdk-stable] [PATCH v8 2/2] hash: flush the rings instead of dequeuing one by one

[Gavin Hu]

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

[dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu]

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

Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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>

Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

[Olivier Matz]

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>

Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

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

Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

[Thomas Monjalon]

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.

Re: [dpdk-stable] [PATCH v9 1/2] ring: add reset API to flush the ring when not in use

[Gavin Hu (Arm Technology China)]

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!

[dpdk-stable] [PATCH v9 2/2] hash: flush the rings instead of dequeuing one by one

[Gavin Hu]

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