Re: [dpdk-dev] [PATCH] event/dsw: use custom element size ring for control

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> Subject: [PATCH] event/dsw: use custom element size ring for control
> 
> Replace DSW's use of regular DPDK rings (and code for packing/unpacking
> control messages into void pointers) with custom size rings.
> 
> In addition to cleaner code, this change allows DSW to support up to the
> eventdev API's maximum of 255 ports by tweaking DSW_MAX_PORTS.
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
>  drivers/event/dsw/Makefile    |  3 +++
>  drivers/event/dsw/dsw_evdev.c |  9 ++++++---
> drivers/event/dsw/dsw_evdev.h | 10 +++-------
> drivers/event/dsw/dsw_event.c | 16 ++--------------
> drivers/event/dsw/meson.build |  3 +++
>  5 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/event/dsw/Makefile b/drivers/event/dsw/Makefile index
> f6e7dda1f..68d681fab 100644
> --- a/drivers/event/dsw/Makefile
> +++ b/drivers/event/dsw/Makefile
> @@ -11,6 +11,9 @@ ifneq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)  CFLAGS += -
> Wno-format-nonliteral  endif
> 
> +# Depends on rte_ring_elem_*()
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +
>  LDLIBS += -lrte_eal
>  LDLIBS += -lrte_mbuf
>  LDLIBS += -lrte_mempool
> diff --git a/drivers/event/dsw/dsw_evdev.c b/drivers/event/dsw/dsw_evdev.c
> index 9387d4149..7798a38ad 100644
> --- a/drivers/event/dsw/dsw_evdev.c
> +++ b/drivers/event/dsw/dsw_evdev.c
> @@ -8,6 +8,7 @@
>  #include <rte_eventdev_pmd.h>
>  #include <rte_eventdev_pmd_vdev.h>
>  #include <rte_random.h>
> +#include <rte_ring_elem.h>
> 
>  #include "dsw_evdev.h"
> 
> @@ -46,9 +47,11 @@ dsw_port_setup(struct rte_eventdev *dev, uint8_t
> port_id,
>  	snprintf(ring_name, sizeof(ring_name), "dswctl%d_p%u",
>  		 dev->data->dev_id, port_id);
> 
> -	ctl_in_ring = rte_ring_create(ring_name, DSW_CTL_IN_RING_SIZE,
> -				      dev->data->socket_id,
> -				      RING_F_SC_DEQ|RING_F_EXACT_SZ);
> +	ctl_in_ring = rte_ring_create_elem(ring_name,
> +					   sizeof(struct dsw_ctl_msg),
> +					   DSW_CTL_IN_RING_SIZE,
> +					   dev->data->socket_id,
> +
> RING_F_SC_DEQ|RING_F_EXACT_SZ);
> 
>  	if (ctl_in_ring == NULL) {
>  		rte_event_ring_free(in_ring);
> diff --git a/drivers/event/dsw/dsw_evdev.h
> b/drivers/event/dsw/dsw_evdev.h index dc28ab125..5c7b6108d 100644
> --- a/drivers/event/dsw/dsw_evdev.h
> +++ b/drivers/event/dsw/dsw_evdev.h
> @@ -10,7 +10,6 @@
> 
>  #define DSW_PMD_NAME RTE_STR(event_dsw)
> 
> -/* Code changes are required to allow more ports. */  #define
> DSW_MAX_PORTS (64)  #define DSW_MAX_PORT_DEQUEUE_DEPTH (128)
> #define DSW_MAX_PORT_ENQUEUE_DEPTH (128) @@ -226,15 +225,12 @@
> struct dsw_evdev {  #define DSW_CTL_UNPAUS_REQ (1)  #define
> DSW_CTL_CFM (2)
> 
> -/* sizeof(struct dsw_ctl_msg) must be equal or less than
> - * sizeof(void *), to fit on the control ring.
> - */
>  struct dsw_ctl_msg {
> -	uint8_t type:2;
> -	uint8_t originating_port_id:6;
> +	uint8_t type;
> +	uint8_t originating_port_id;
>  	uint8_t queue_id;
>  	uint16_t flow_hash;
> -} __rte_packed;
> +} __rte_aligned(4);
Minor comment, would it be possible to separate this into another commit?

> 
>  uint16_t dsw_event_enqueue(void *port, const struct rte_event *event);
> uint16_t dsw_event_enqueue_burst(void *port, diff --git
> a/drivers/event/dsw/dsw_event.c b/drivers/event/dsw/dsw_event.c index
> eae53b240..d68b71b98 100644
> --- a/drivers/event/dsw/dsw_event.c
> +++ b/drivers/event/dsw/dsw_event.c
> @@ -176,27 +176,15 @@ dsw_port_consider_load_update(struct dsw_port
> *port, uint64_t now)  static void  dsw_port_ctl_enqueue(struct dsw_port
> *port, struct dsw_ctl_msg *msg)  {
> -	void *raw_msg;
> -
> -	memcpy(&raw_msg, msg, sizeof(*msg));
> -
>  	/* there's always room on the ring */
> -	while (rte_ring_enqueue(port->ctl_in_ring, raw_msg) != 0)
> +	while (rte_ring_enqueue_elem(port->ctl_in_ring, msg,
> sizeof(*msg)) !=
> +0)
>  		rte_pause();
>  }
> 
>  static int
>  dsw_port_ctl_dequeue(struct dsw_port *port, struct dsw_ctl_msg *msg)  {
> -	void *raw_msg;
> -	int rc;
> -
> -	rc = rte_ring_dequeue(port->ctl_in_ring, &raw_msg);
> -
> -	if (rc == 0)
> -		memcpy(msg, &raw_msg, sizeof(*msg));
> -
> -	return rc;
> +	return rte_ring_dequeue_elem(port->ctl_in_ring, msg, sizeof(*msg));
>  }
> 
>  static void
> diff --git a/drivers/event/dsw/meson.build b/drivers/event/dsw/meson.build
> index 60ab13d90..3b39cb653 100644
> --- a/drivers/event/dsw/meson.build
> +++ b/drivers/event/dsw/meson.build
> @@ -6,3 +6,6 @@ if cc.has_argument('-Wno-format-nonliteral')
>  	cflags += '-Wno-format-nonliteral'
>  endif
>  sources = files('dsw_evdev.c', 'dsw_event.c', 'dsw_xstats.c')
> +
> +# Depends on rte_ring_elem_*()
> +allow_experimental_apis = true
The rte_ring_elem_xxx API changes look good.

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

> --
> 2.17.1