From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 1F9B1E5D for ; Mon, 11 Dec 2017 13:36:08 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2017 04:36:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,391,1508828400"; d="scan'208";a="532531" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga002.fm.intel.com with ESMTP; 11 Dec 2017 04:36:06 -0800 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.180]) by IRSMSX103.ger.corp.intel.com ([169.254.3.49]) with mapi id 14.03.0319.002; Mon, 11 Dec 2017 12:36:06 +0000 From: "Van Haaren, Harry" To: "Eads, Gage" , "dev@dpdk.org" CC: "jerin.jacob@caviumnetworks.com" , "Richardson, Bruce" , "hemant.agrawal@nxp.com" , "nipun.gupta@nxp.com" , "santosh.shukla@caviumnetworks.com" Thread-Topic: [PATCH 1/2] eventdev: add implicit release disable capability Thread-Index: AQHTaZKeq/Fogkz+qU+t5EfC+lDk9KM+G/Mw Date: Mon, 11 Dec 2017 12:36:05 +0000 Message-ID: References: <1512015636-31878-1-git-send-email-gage.eads@intel.com> <1512015636-31878-2-git-send-email-gage.eads@intel.com> In-Reply-To: <1512015636-31878-2-git-send-email-gage.eads@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTEwZDAxMDEtMmViNC00NmY0LWFiZmMtZjdmZDg4NjVkNjQ3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6InNSSnlyWmtTTnZldnNkSHlzWnZwT2ZWOSt1bHowRkNnaGg4MnN3KzlZZ2s9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/2] eventdev: add implicit release disable capability X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Dec 2017 12:36:09 -0000 > -----Original Message----- > From: Eads, Gage > Sent: Thursday, November 30, 2017 4:21 AM > To: dev@dpdk.org > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry > ; Richardson, Bruce > ; hemant.agrawal@nxp.com; nipun.gupta@nxp.com= ; > santosh.shukla@caviumnetworks.com > Subject: [PATCH 1/2] eventdev: add implicit release disable capability >=20 > 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. >=20 > If a PMD does not support this capability, the application will receive a= n > 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. >=20 > Signed-off-by: Gage Eads Some comments inline. In general, I think this makes sense, and is the easi= est 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(-) >=20 > 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 =3D > DPAA2_EVENT_MAX_PORT_ENQUEUE_DEPTH; > + port_conf->disable_implicit_release =3D > + 0; Merge "0;" onto previous line? > --- 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 =3D 32 * 1024; > port_conf->dequeue_depth =3D 16; > port_conf->enqueue_depth =3D 16; > + port_conf->disable_implicit_release =3D false; Prefer 0 to false. > 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 . I = haven't seen stdbool.h in other DPDK headers, so suggesting stick with the = good old byte-sized integers for flags..=20 > } __rte_cache_aligned; >=20 > static struct prod_data prod_data; > @@ -167,6 +168,18 @@ consumer(void) > uint8_t outport =3D packets[i].mbuf->port; > rte_eth_tx_buffer(outport, 0, fdata->tx_buf[outport], > packets[i].mbuf); > + > + packets[i].op =3D RTE_EVENT_OP_RELEASE; > + } > + > + if (cons_data.release) { > + uint16_t nb_tx; > + > + nb_tx =3D rte_event_enqueue_burst(dev_id, port_id, packets, n); > + while (nb_tx < n) > + nb_tx +=3D rte_event_enqueue_burst(dev_id, port_id, > + packets + nb_tx, > + n - nb_tx); > } >=20 > /* Print out mpps every 1<22 packets */ > @@ -702,6 +715,7 @@ setup_eventdev(struct prod_data *prod_data, > }; >=20 > 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() >=3D 3) { > printf("*** Running Worker loopback test...\n"); > - ret =3D worker_loopback(t); > + ret =3D worker_loopback(t, 0); > if (ret !=3D 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() >=3D 3) { > + printf("*** Running Worker loopback test (implicit release > disabled)...\n"); > + ret =3D worker_loopback(t, 1); > + if (ret !=3D 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 >=3D 3) here looks like it could be removed and the lo= opback(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