From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id E6B072A5D for ; Mon, 11 Dec 2017 18:39:33 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2017 09:39:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,392,1508828400"; d="scan'208";a="1586151" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga007.fm.intel.com with ESMTP; 11 Dec 2017 09:39:32 -0800 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 11 Dec 2017 09:39:32 -0800 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.23]) by FMSMSX157.amr.corp.intel.com ([169.254.14.145]) with mapi id 14.03.0319.002; Mon, 11 Dec 2017 09:39:32 -0800 From: "Eads, Gage" To: "Van Haaren, Harry" , "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/MwgABeyUA= Date: Mon, 11 Dec 2017 17:39:31 +0000 Message-ID: <9184057F7FC11744A2107296B6B8EB1E2BB164EB@FMSMSX108.amr.corp.intel.com> References: <1512015636-31878-1-git-send-email-gage.eads@intel.com> <1512015636-31878-2-git-send-email-gage.eads@intel.com> In-Reply-To: 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: [10.1.200.106] 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 17:39:34 -0000 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 ; dev@dpdk.org > Cc: jerin.jacob@caviumnetworks.com; Richardson, Bruce > ; hemant.agrawal@nxp.com; > nipun.gupta@nxp.com; santosh.shukla@caviumnetworks.com > Subject: RE: [PATCH 1/2] eventdev: add implicit release disable capabilit= y >=20 > > -----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 > > > > 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 >=20 > Some comments inline. In general, I think this makes sense, and is the ea= siest > solution that I can see. >=20 >=20 > > 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 =3D > > DPAA2_EVENT_MAX_PORT_ENQUEUE_DEPTH; > > + port_conf->disable_implicit_release =3D > > + 0; >=20 > Merge "0;" onto previous line? >=20 > >=20 > > --- 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; >=20 > Prefer 0 to false. >=20 > >=20 > > 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; >=20 > 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 o= ld > byte-sized integers for flags.. >=20 >=20 > > } __rte_cache_aligned; > > > > 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); > > } > > > > /* 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; >=20 > Same uint8_t over stdbool.h comment as above >=20 >=20 > > @@ -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"); > > + } >=20 > The double if (count >=3D 3) here looks like it could be removed and the > loopback(t, 1) added to the first if()- but otherwise the logic is fine. >=20 >=20 > With the above changes, this looks good to me! >=20 > Acked-by: Harry van Haaren