event/dsw: use custom element size ring for control
Checks
Commit Message
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(-)
Comments
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mattias Rönnblom
> Sent: Monday, January 20, 2020 4:03 PM
>
> 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.
Considering that the mbuf port_id was increased from 8 to 16 bit a long time ago... does it make sense increasing this and perhaps the eventdev API's type from 8 to 16 bit too?
>
> 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(-)
>
[snip]
> 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)
64 or 256?
> #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);
[snip]
Med venlig hilsen / kind regards
- Morten Brørup
On 2020-01-21 13:04, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mattias Rönnblom
>> Sent: Monday, January 20, 2020 4:03 PM
>>
>> 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.
> Considering that the mbuf port_id was increased from 8 to 16 bit a long time ago... does it make sense increasing this and perhaps the eventdev API's type from 8 to 16 bit too?
Moving from uint8_t to uint16_t in the Eventdev API seems like a good
idea to me. The need for eventdev ports typically grows with growing
lcore count, and some systems aren't too far from 255 (or is it 256)?
I don't think it makes sense to use uint16_t internally in DSW before
this happen, though.
>> 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(-)
>>
> [snip]
>
>> 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)
> 64 or 256?
64 to save some memory, in the common case.
>> #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);
> [snip]
>
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
<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
On Wed, Jan 22, 2020 at 10:36 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <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>
> > +
> > +# 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>
Applied to dpdk-next-eventdev/master. Thanks.
>
> > --
> > 2.17.1
>
@@ -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
@@ -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);
@@ -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);
uint16_t dsw_event_enqueue(void *port, const struct rte_event *event);
uint16_t dsw_event_enqueue_burst(void *port,
@@ -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
@@ -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