[v9,6/6] lib/eventdev: use custom element size ring for event rings

Message ID 20200116052511.8557-7-honnappa.nagarahalli@arm.com
State Superseded
Headers show
Series
  • lib/ring: APIs to support custom element size
Related show

Commit Message

Honnappa Nagarahalli Jan. 16, 2020, 5:25 a.m.
Use custom element size ring APIs to replace event ring
implementation. This avoids code duplication.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

Reviewed-by: Gavin Hu <gavin.hu@arm.com>

Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>

---
 lib/librte_eventdev/rte_event_ring.c | 147 ++-------------------------
 lib/librte_eventdev/rte_event_ring.h |  45 ++++----
 2 files changed, 24 insertions(+), 168 deletions(-)

-- 
2.17.1

Comments

Jerin Jacob Jan. 17, 2020, 2:41 p.m. | #1
On Thu, Jan 16, 2020 at 10:56 AM Honnappa Nagarahalli
<honnappa.nagarahalli@arm.com> wrote:
>

> Use custom element size ring APIs to replace event ring

> implementation. This avoids code duplication.

>

> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> Reviewed-by: Gavin Hu <gavin.hu@arm.com>

> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>


Please change the subject to eventdev: xxxxxx.

With the above change, LGTM

Reviewed-by: Jerin Jacob <jerinj@marvell.com>


> ---

>  lib/librte_eventdev/rte_event_ring.c | 147 ++-------------------------

>  lib/librte_eventdev/rte_event_ring.h |  45 ++++----

>  2 files changed, 24 insertions(+), 168 deletions(-)

>

> diff --git a/lib/librte_eventdev/rte_event_ring.c b/lib/librte_eventdev/rte_event_ring.c

> index 50190de01..d27e23901 100644

> --- a/lib/librte_eventdev/rte_event_ring.c

> +++ b/lib/librte_eventdev/rte_event_ring.c

> @@ -1,5 +1,6 @@

>  /* SPDX-License-Identifier: BSD-3-Clause

>   * Copyright(c) 2017 Intel Corporation

> + * Copyright(c) 2019 Arm Limited

>   */

>

>  #include <sys/queue.h>

> @@ -11,13 +12,6 @@

>  #include <rte_eal_memconfig.h>

>  #include "rte_event_ring.h"

>

> -TAILQ_HEAD(rte_event_ring_list, rte_tailq_entry);

> -

> -static struct rte_tailq_elem rte_event_ring_tailq = {

> -       .name = RTE_TAILQ_EVENT_RING_NAME,

> -};

> -EAL_REGISTER_TAILQ(rte_event_ring_tailq)

> -

>  int

>  rte_event_ring_init(struct rte_event_ring *r, const char *name,

>         unsigned int count, unsigned int flags)

> @@ -35,150 +29,21 @@ struct rte_event_ring *

>  rte_event_ring_create(const char *name, unsigned int count, int socket_id,

>                 unsigned int flags)

>  {

> -       char mz_name[RTE_MEMZONE_NAMESIZE];

> -       struct rte_event_ring *r;

> -       struct rte_tailq_entry *te;

> -       const struct rte_memzone *mz;

> -       ssize_t ring_size;

> -       int mz_flags = 0;

> -       struct rte_event_ring_list *ring_list = NULL;

> -       const unsigned int requested_count = count;

> -       int ret;

> -

> -       ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,

> -               rte_event_ring_list);

> -

> -       /* for an exact size ring, round up from count to a power of two */

> -       if (flags & RING_F_EXACT_SZ)

> -               count = rte_align32pow2(count + 1);

> -       else if (!rte_is_power_of_2(count)) {

> -               rte_errno = EINVAL;

> -               return NULL;

> -       }

> -

> -       ring_size = sizeof(*r) + (count * sizeof(struct rte_event));

> -

> -       ret = snprintf(mz_name, sizeof(mz_name), "%s%s",

> -               RTE_RING_MZ_PREFIX, name);

> -       if (ret < 0 || ret >= (int)sizeof(mz_name)) {

> -               rte_errno = ENAMETOOLONG;

> -               return NULL;

> -       }

> -

> -       te = rte_zmalloc("RING_TAILQ_ENTRY", sizeof(*te), 0);

> -       if (te == NULL) {

> -               RTE_LOG(ERR, RING, "Cannot reserve memory for tailq\n");

> -               rte_errno = ENOMEM;

> -               return NULL;

> -       }

> -

> -       rte_mcfg_tailq_write_lock();

> -

> -       /*

> -        * reserve a memory zone for this ring. If we can't get rte_config or

> -        * we are secondary process, the memzone_reserve function will set

> -        * rte_errno for us appropriately - hence no check in this this function

> -        */

> -       mz = rte_memzone_reserve(mz_name, ring_size, socket_id, mz_flags);

> -       if (mz != NULL) {

> -               r = mz->addr;

> -               /* Check return value in case rte_ring_init() fails on size */

> -               int err = rte_event_ring_init(r, name, requested_count, flags);

> -               if (err) {

> -                       RTE_LOG(ERR, RING, "Ring init failed\n");

> -                       if (rte_memzone_free(mz) != 0)

> -                               RTE_LOG(ERR, RING, "Cannot free memzone\n");

> -                       rte_free(te);

> -                       rte_mcfg_tailq_write_unlock();

> -                       return NULL;

> -               }

> -

> -               te->data = (void *) r;

> -               r->r.memzone = mz;

> -

> -               TAILQ_INSERT_TAIL(ring_list, te, next);

> -       } else {

> -               r = NULL;

> -               RTE_LOG(ERR, RING, "Cannot reserve memory\n");

> -               rte_free(te);

> -       }

> -       rte_mcfg_tailq_write_unlock();

> -

> -       return r;

> +       return (struct rte_event_ring *)rte_ring_create_elem(name,

> +                                               sizeof(struct rte_event),

> +                                               count, socket_id, flags);

>  }

>

>

>  struct rte_event_ring *

>  rte_event_ring_lookup(const char *name)

>  {

> -       struct rte_tailq_entry *te;

> -       struct rte_event_ring *r = NULL;

> -       struct rte_event_ring_list *ring_list;

> -

> -       ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,

> -                       rte_event_ring_list);

> -

> -       rte_mcfg_tailq_read_lock();

> -

> -       TAILQ_FOREACH(te, ring_list, next) {

> -               r = (struct rte_event_ring *) te->data;

> -               if (strncmp(name, r->r.name, RTE_RING_NAMESIZE) == 0)

> -                       break;

> -       }

> -

> -       rte_mcfg_tailq_read_unlock();

> -

> -       if (te == NULL) {

> -               rte_errno = ENOENT;

> -               return NULL;

> -       }

> -

> -       return r;

> +       return (struct rte_event_ring *)rte_ring_lookup(name);

>  }

>

>  /* free the ring */

>  void

>  rte_event_ring_free(struct rte_event_ring *r)

>  {

> -       struct rte_event_ring_list *ring_list = NULL;

> -       struct rte_tailq_entry *te;

> -

> -       if (r == NULL)

> -               return;

> -

> -       /*

> -        * Ring was not created with rte_event_ring_create,

> -        * therefore, there is no memzone to free.

> -        */

> -       if (r->r.memzone == NULL) {

> -               RTE_LOG(ERR, RING,

> -                       "Cannot free ring (not created with rte_event_ring_create()");

> -               return;

> -       }

> -

> -       if (rte_memzone_free(r->r.memzone) != 0) {

> -               RTE_LOG(ERR, RING, "Cannot free memory\n");

> -               return;

> -       }

> -

> -       ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,

> -                       rte_event_ring_list);

> -       rte_mcfg_tailq_write_lock();

> -

> -       /* find out tailq entry */

> -       TAILQ_FOREACH(te, ring_list, next) {

> -               if (te->data == (void *) r)

> -                       break;

> -       }

> -

> -       if (te == NULL) {

> -               rte_mcfg_tailq_write_unlock();

> -               return;

> -       }

> -

> -       TAILQ_REMOVE(ring_list, te, next);

> -

> -       rte_mcfg_tailq_write_unlock();

> -

> -       rte_free(te);

> +       rte_ring_free((struct rte_ring *)r);

>  }

> diff --git a/lib/librte_eventdev/rte_event_ring.h b/lib/librte_eventdev/rte_event_ring.h

> index 827a3209e..c0861b0ec 100644

> --- a/lib/librte_eventdev/rte_event_ring.h

> +++ b/lib/librte_eventdev/rte_event_ring.h

> @@ -1,5 +1,6 @@

>  /* SPDX-License-Identifier: BSD-3-Clause

>   * Copyright(c) 2016-2017 Intel Corporation

> + * Copyright(c) 2019 Arm Limited

>   */

>

>  /**

> @@ -19,6 +20,7 @@

>  #include <rte_memory.h>

>  #include <rte_malloc.h>

>  #include <rte_ring.h>

> +#include <rte_ring_elem.h>

>  #include "rte_eventdev.h"

>

>  #define RTE_TAILQ_EVENT_RING_NAME "RTE_EVENT_RING"

> @@ -88,22 +90,17 @@ rte_event_ring_enqueue_burst(struct rte_event_ring *r,

>                 const struct rte_event *events,

>                 unsigned int n, uint16_t *free_space)

>  {

> -       uint32_t prod_head, prod_next;

> -       uint32_t free_entries;

> +       unsigned int num;

> +       uint32_t space;

>

> -       n = __rte_ring_move_prod_head(&r->r, r->r.prod.single, n,

> -                       RTE_RING_QUEUE_VARIABLE,

> -                       &prod_head, &prod_next, &free_entries);

> -       if (n == 0)

> -               goto end;

> +       num = rte_ring_enqueue_burst_elem(&r->r, events,

> +                               sizeof(struct rte_event), n,

> +                               &space);

>

> -       ENQUEUE_PTRS(&r->r, &r[1], prod_head, events, n, struct rte_event);

> -

> -       update_tail(&r->r.prod, prod_head, prod_next, r->r.prod.single, 1);

> -end:

>         if (free_space != NULL)

> -               *free_space = free_entries - n;

> -       return n;

> +               *free_space = space;

> +

> +       return num;

>  }

>

>  /**

> @@ -129,23 +126,17 @@ rte_event_ring_dequeue_burst(struct rte_event_ring *r,

>                 struct rte_event *events,

>                 unsigned int n, uint16_t *available)

>  {

> -       uint32_t cons_head, cons_next;

> -       uint32_t entries;

> -

> -       n = __rte_ring_move_cons_head(&r->r, r->r.cons.single, n,

> -                       RTE_RING_QUEUE_VARIABLE,

> -                       &cons_head, &cons_next, &entries);

> -       if (n == 0)

> -               goto end;

> +       unsigned int num;

> +       uint32_t remaining;

>

> -       DEQUEUE_PTRS(&r->r, &r[1], cons_head, events, n, struct rte_event);

> +       num = rte_ring_dequeue_burst_elem(&r->r, events,

> +                               sizeof(struct rte_event), n,

> +                               &remaining);

>

> -       update_tail(&r->r.cons, cons_head, cons_next, r->r.cons.single, 0);

> -

> -end:

>         if (available != NULL)

> -               *available = entries - n;

> -       return n;

> +               *available = remaining;

> +

> +       return num;

>  }

>

>  /*

> --

> 2.17.1

>
David Marchand Jan. 17, 2020, 4:12 p.m. | #2
On Fri, Jan 17, 2020 at 3:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>

> On Thu, Jan 16, 2020 at 10:56 AM Honnappa Nagarahalli

> <honnappa.nagarahalli@arm.com> wrote:

> >

> > Use custom element size ring APIs to replace event ring

> > implementation. This avoids code duplication.

> >

> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>

> > Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>

>

> Please change the subject to eventdev: xxxxxx.


I can do while applying.

>

> With the above change, LGTM

>

> Reviewed-by: Jerin Jacob <jerinj@marvell.com>


Thanks Jerin.


--
David Marchand

Patch

diff --git a/lib/librte_eventdev/rte_event_ring.c b/lib/librte_eventdev/rte_event_ring.c
index 50190de01..d27e23901 100644
--- a/lib/librte_eventdev/rte_event_ring.c
+++ b/lib/librte_eventdev/rte_event_ring.c
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2017 Intel Corporation
+ * Copyright(c) 2019 Arm Limited
  */
 
 #include <sys/queue.h>
@@ -11,13 +12,6 @@ 
 #include <rte_eal_memconfig.h>
 #include "rte_event_ring.h"
 
-TAILQ_HEAD(rte_event_ring_list, rte_tailq_entry);
-
-static struct rte_tailq_elem rte_event_ring_tailq = {
-	.name = RTE_TAILQ_EVENT_RING_NAME,
-};
-EAL_REGISTER_TAILQ(rte_event_ring_tailq)
-
 int
 rte_event_ring_init(struct rte_event_ring *r, const char *name,
 	unsigned int count, unsigned int flags)
@@ -35,150 +29,21 @@  struct rte_event_ring *
 rte_event_ring_create(const char *name, unsigned int count, int socket_id,
 		unsigned int flags)
 {
-	char mz_name[RTE_MEMZONE_NAMESIZE];
-	struct rte_event_ring *r;
-	struct rte_tailq_entry *te;
-	const struct rte_memzone *mz;
-	ssize_t ring_size;
-	int mz_flags = 0;
-	struct rte_event_ring_list *ring_list = NULL;
-	const unsigned int requested_count = count;
-	int ret;
-
-	ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,
-		rte_event_ring_list);
-
-	/* for an exact size ring, round up from count to a power of two */
-	if (flags & RING_F_EXACT_SZ)
-		count = rte_align32pow2(count + 1);
-	else if (!rte_is_power_of_2(count)) {
-		rte_errno = EINVAL;
-		return NULL;
-	}
-
-	ring_size = sizeof(*r) + (count * sizeof(struct rte_event));
-
-	ret = snprintf(mz_name, sizeof(mz_name), "%s%s",
-		RTE_RING_MZ_PREFIX, name);
-	if (ret < 0 || ret >= (int)sizeof(mz_name)) {
-		rte_errno = ENAMETOOLONG;
-		return NULL;
-	}
-
-	te = rte_zmalloc("RING_TAILQ_ENTRY", sizeof(*te), 0);
-	if (te == NULL) {
-		RTE_LOG(ERR, RING, "Cannot reserve memory for tailq\n");
-		rte_errno = ENOMEM;
-		return NULL;
-	}
-
-	rte_mcfg_tailq_write_lock();
-
-	/*
-	 * reserve a memory zone for this ring. If we can't get rte_config or
-	 * we are secondary process, the memzone_reserve function will set
-	 * rte_errno for us appropriately - hence no check in this this function
-	 */
-	mz = rte_memzone_reserve(mz_name, ring_size, socket_id, mz_flags);
-	if (mz != NULL) {
-		r = mz->addr;
-		/* Check return value in case rte_ring_init() fails on size */
-		int err = rte_event_ring_init(r, name, requested_count, flags);
-		if (err) {
-			RTE_LOG(ERR, RING, "Ring init failed\n");
-			if (rte_memzone_free(mz) != 0)
-				RTE_LOG(ERR, RING, "Cannot free memzone\n");
-			rte_free(te);
-			rte_mcfg_tailq_write_unlock();
-			return NULL;
-		}
-
-		te->data = (void *) r;
-		r->r.memzone = mz;
-
-		TAILQ_INSERT_TAIL(ring_list, te, next);
-	} else {
-		r = NULL;
-		RTE_LOG(ERR, RING, "Cannot reserve memory\n");
-		rte_free(te);
-	}
-	rte_mcfg_tailq_write_unlock();
-
-	return r;
+	return (struct rte_event_ring *)rte_ring_create_elem(name,
+						sizeof(struct rte_event),
+						count, socket_id, flags);
 }
 
 
 struct rte_event_ring *
 rte_event_ring_lookup(const char *name)
 {
-	struct rte_tailq_entry *te;
-	struct rte_event_ring *r = NULL;
-	struct rte_event_ring_list *ring_list;
-
-	ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,
-			rte_event_ring_list);
-
-	rte_mcfg_tailq_read_lock();
-
-	TAILQ_FOREACH(te, ring_list, next) {
-		r = (struct rte_event_ring *) te->data;
-		if (strncmp(name, r->r.name, RTE_RING_NAMESIZE) == 0)
-			break;
-	}
-
-	rte_mcfg_tailq_read_unlock();
-
-	if (te == NULL) {
-		rte_errno = ENOENT;
-		return NULL;
-	}
-
-	return r;
+	return (struct rte_event_ring *)rte_ring_lookup(name);
 }
 
 /* free the ring */
 void
 rte_event_ring_free(struct rte_event_ring *r)
 {
-	struct rte_event_ring_list *ring_list = NULL;
-	struct rte_tailq_entry *te;
-
-	if (r == NULL)
-		return;
-
-	/*
-	 * Ring was not created with rte_event_ring_create,
-	 * therefore, there is no memzone to free.
-	 */
-	if (r->r.memzone == NULL) {
-		RTE_LOG(ERR, RING,
-			"Cannot free ring (not created with rte_event_ring_create()");
-		return;
-	}
-
-	if (rte_memzone_free(r->r.memzone) != 0) {
-		RTE_LOG(ERR, RING, "Cannot free memory\n");
-		return;
-	}
-
-	ring_list = RTE_TAILQ_CAST(rte_event_ring_tailq.head,
-			rte_event_ring_list);
-	rte_mcfg_tailq_write_lock();
-
-	/* find out tailq entry */
-	TAILQ_FOREACH(te, ring_list, next) {
-		if (te->data == (void *) r)
-			break;
-	}
-
-	if (te == NULL) {
-		rte_mcfg_tailq_write_unlock();
-		return;
-	}
-
-	TAILQ_REMOVE(ring_list, te, next);
-
-	rte_mcfg_tailq_write_unlock();
-
-	rte_free(te);
+	rte_ring_free((struct rte_ring *)r);
 }
diff --git a/lib/librte_eventdev/rte_event_ring.h b/lib/librte_eventdev/rte_event_ring.h
index 827a3209e..c0861b0ec 100644
--- a/lib/librte_eventdev/rte_event_ring.h
+++ b/lib/librte_eventdev/rte_event_ring.h
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright(c) 2016-2017 Intel Corporation
+ * Copyright(c) 2019 Arm Limited
  */
 
 /**
@@ -19,6 +20,7 @@ 
 #include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_ring.h>
+#include <rte_ring_elem.h>
 #include "rte_eventdev.h"
 
 #define RTE_TAILQ_EVENT_RING_NAME "RTE_EVENT_RING"
@@ -88,22 +90,17 @@  rte_event_ring_enqueue_burst(struct rte_event_ring *r,
 		const struct rte_event *events,
 		unsigned int n, uint16_t *free_space)
 {
-	uint32_t prod_head, prod_next;
-	uint32_t free_entries;
+	unsigned int num;
+	uint32_t space;
 
-	n = __rte_ring_move_prod_head(&r->r, r->r.prod.single, n,
-			RTE_RING_QUEUE_VARIABLE,
-			&prod_head, &prod_next, &free_entries);
-	if (n == 0)
-		goto end;
+	num = rte_ring_enqueue_burst_elem(&r->r, events,
+				sizeof(struct rte_event), n,
+				&space);
 
-	ENQUEUE_PTRS(&r->r, &r[1], prod_head, events, n, struct rte_event);
-
-	update_tail(&r->r.prod, prod_head, prod_next, r->r.prod.single, 1);
-end:
 	if (free_space != NULL)
-		*free_space = free_entries - n;
-	return n;
+		*free_space = space;
+
+	return num;
 }
 
 /**
@@ -129,23 +126,17 @@  rte_event_ring_dequeue_burst(struct rte_event_ring *r,
 		struct rte_event *events,
 		unsigned int n, uint16_t *available)
 {
-	uint32_t cons_head, cons_next;
-	uint32_t entries;
-
-	n = __rte_ring_move_cons_head(&r->r, r->r.cons.single, n,
-			RTE_RING_QUEUE_VARIABLE,
-			&cons_head, &cons_next, &entries);
-	if (n == 0)
-		goto end;
+	unsigned int num;
+	uint32_t remaining;
 
-	DEQUEUE_PTRS(&r->r, &r[1], cons_head, events, n, struct rte_event);
+	num = rte_ring_dequeue_burst_elem(&r->r, events,
+				sizeof(struct rte_event), n,
+				&remaining);
 
-	update_tail(&r->r.cons, cons_head, cons_next, r->r.cons.single, 0);
-
-end:
 	if (available != NULL)
-		*available = entries - n;
-	return n;
+		*available = remaining;
+
+	return num;
 }
 
 /*