> -----Original Message-----
> From: Eads, Gage
> Sent: Thursday, November 30, 2017 4:21 AM
> To: dev@dpdk.org
> Cc: jerin.ja...@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haa...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; hemant.agra...@nxp.com; nipun.gu...@nxp.com;
> santosh.shu...@caviumnetworks.com
> Subject: [PATCH 1/2] eventdev: add implicit release disable capability
>
> This commit introduces a capability for disabling the "implicit" release
> functionality for a port, which prevents the eventdev PMD from issuing
> outstanding releases for previously dequeued events when dequeuing a new
> batch of events.
>
> If a PMD does not support this capability, the application will receive an
> error if it attempts to setup a port with implicit releases disabled.
> Otherwise, if the port is configured with implicit releases disabled, the
> application must release each dequeued event by invoking
> rte_event_enqueue_burst() with RTE_EVENT_OP_RELEASE or
> RTE_EVENT_OP_FORWARD.
>
> Signed-off-by: Gage Eads <gage.e...@intel.com>
Some comments inline. In general, I think this makes sense, and is the easiest
solution that I can see.
> drivers/event/dpaa2/dpaa2_eventdev.c | 2 ++
> drivers/event/octeontx/ssovf_evdev.c | 1 +
> drivers/event/skeleton/skeleton_eventdev.c | 1 +
> drivers/event/sw/sw_evdev.c | 10 +++++++---
> drivers/event/sw/sw_evdev.h | 1 +
> drivers/event/sw/sw_evdev_worker.c | 16 ++++++++--------
> examples/eventdev_pipeline_sw_pmd/main.c | 24 +++++++++++++++++++++++-
> lib/librte_eventdev/rte_eventdev.c | 9 +++++++++
> lib/librte_eventdev/rte_eventdev.h | 23 ++++++++++++++++++++---
> test/test/test_eventdev.c | 9 +++++++++
> test/test/test_eventdev_sw.c | 20 ++++++++++++++++++--
> 11 files changed, 99 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/event/dpaa2/dpaa2_eventdev.c
> b/drivers/event/dpaa2/dpaa2_eventdev.c
> index eeeb231..236b211 100644
> --- a/drivers/event/dpaa2/dpaa2_eventdev.c
> +++ b/drivers/event/dpaa2/dpaa2_eventdev.c
> @@ -440,6 +440,8 @@ dpaa2_eventdev_port_def_conf(struct rte_eventdev *dev,
> uint8_t port_id,
> DPAA2_EVENT_MAX_PORT_DEQUEUE_DEPTH;
> port_conf->enqueue_depth =
> DPAA2_EVENT_MAX_PORT_ENQUEUE_DEPTH;
> + port_conf->disable_implicit_release =
> + 0;
Merge "0;" onto previous line?
<snip>
> --- a/drivers/event/skeleton/skeleton_eventdev.c
> +++ b/drivers/event/skeleton/skeleton_eventdev.c
> @@ -237,6 +237,7 @@ skeleton_eventdev_port_def_conf(struct rte_eventdev
> *dev, uint8_t port_id,
> port_conf->new_event_threshold = 32 * 1024;
> port_conf->dequeue_depth = 16;
> port_conf->enqueue_depth = 16;
> + port_conf->disable_implicit_release = false;
Prefer 0 to false.
<snip>
> diff --git a/examples/eventdev_pipeline_sw_pmd/main.c
> b/examples/eventdev_pipeline_sw_pmd/main.c
> index 5f431d8..3910b53 100644
> --- a/examples/eventdev_pipeline_sw_pmd/main.c
> +++ b/examples/eventdev_pipeline_sw_pmd/main.c
> @@ -62,6 +62,7 @@ struct prod_data {
> struct cons_data {
> uint8_t dev_id;
> uint8_t port_id;
> + bool release;
I'd personally uint8_t this instead of bool, which requires <stdbool.h>. I
haven't seen stdbool.h in other DPDK headers, so suggesting stick with the good
old byte-sized integers for flags..
> } __rte_cache_aligned;
>
> static struct prod_data prod_data;
> @@ -167,6 +168,18 @@ consumer(void)
> uint8_t outport = packets[i].mbuf->port;
> rte_eth_tx_buffer(outport, 0, fdata->tx_buf[outport],
> packets[i].mbuf);
> +
> + packets[i].op = RTE_EVENT_OP_RELEASE;
> + }
> +
> + if (cons_data.release) {
> + uint16_t nb_tx;
> +
> + nb_tx = rte_event_enqueue_burst(dev_id, port_id, packets, n);
> + while (nb_tx < n)
> + nb_tx += rte_event_enqueue_burst(dev_id, port_id,
> + packets + nb_tx,
> + n - nb_tx);
> }
>
> /* Print out mpps every 1<22 packets */
> @@ -702,6 +715,7 @@ setup_eventdev(struct prod_data *prod_data,
> };
>
> struct port_link worker_queues[MAX_NUM_STAGES];
> + bool disable_implicit_release;
Same uint8_t over stdbool.h comment as above
> @@ -3240,7 +3244,7 @@ test_sw_eventdev(void)
> if (rte_lcore_count() >= 3) {
> printf("*** Running Worker loopback test...\n");
> - ret = worker_loopback(t);
> + ret = worker_loopback(t, 0);
> if (ret != 0) {
> printf("ERROR - Worker loopback test FAILED.\n");
> return ret;
> @@ -3249,6 +3253,18 @@ test_sw_eventdev(void)
> printf("### Not enough cores for worker loopback test.\n");
> printf("### Need at least 3 cores for test.\n");
> }
> + if (rte_lcore_count() >= 3) {
> + printf("*** Running Worker loopback test (implicit release
> disabled)...\n");
> + ret = worker_loopback(t, 1);
> + if (ret != 0) {
> + printf("ERROR - Worker loopback test FAILED.\n");
> + return ret;
> + }
> + } else {
> + printf("### Not enough cores for worker loopback test.\n");
> + printf("### Need at least 3 cores for test.\n");
> + }
The double if (count >= 3) here looks like it could be removed and the
loopback(t, 1) added to the first if()- but otherwise the logic is fine.
With the above changes, this looks good to me!
Acked-by: Harry van Haaren <harry.van.haa...@intel.com>
