Sure, will make all suggested changes in v2.
> -----Original Message----- > From: Van Haaren, Harry > Sent: Monday, December 11, 2017 6:36 AM > To: Eads, Gage <gage.e...@intel.com>; dev@dpdk.org > Cc: jerin.ja...@caviumnetworks.com; Richardson, Bruce > <bruce.richard...@intel.com>; hemant.agra...@nxp.com; > nipun.gu...@nxp.com; santosh.shu...@caviumnetworks.com > Subject: RE: [PATCH 1/2] eventdev: add implicit release disable capability > > > -----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>