DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Eads, Gage" <gage.eads@intel.com>
To: "Van Haaren, Harry" <harry.van.haaren@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	"nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
	"santosh.shukla@caviumnetworks.com"
	<santosh.shukla@caviumnetworks.com>
Subject: Re: [dpdk-dev] [PATCH 1/2] eventdev: add implicit release disable capability
Date: Mon, 11 Dec 2017 17:39:31 +0000	[thread overview]
Message-ID: <9184057F7FC11744A2107296B6B8EB1E2BB164EB@FMSMSX108.amr.corp.intel.com> (raw)
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA650FE00EA@IRSMSX102.ger.corp.intel.com>

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.eads@intel.com>; dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
> nipun.gupta@nxp.com; santosh.shukla@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.jacob@caviumnetworks.com; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; 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 <gage.eads@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.haaren@intel.com>

  reply	other threads:[~2017-12-11 17:39 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-30  4:20 [dpdk-dev] [PATCH 0/2] " Gage Eads
2017-11-30  4:20 ` [dpdk-dev] [PATCH 1/2] eventdev: " Gage Eads
2017-12-11 12:36   ` Van Haaren, Harry
2017-12-11 17:39     ` Eads, Gage [this message]
2017-12-11 17:56   ` [dpdk-dev] [PATCH v2 " Gage Eads
2017-12-11 17:56     ` [dpdk-dev] [PATCH v2 2/2] event/sw: simplify credit scheme Gage Eads
2017-12-16  8:50     ` [dpdk-dev] [PATCH v2 1/2] eventdev: add implicit release disable capability Jerin Jacob
2017-11-30  4:20 ` [dpdk-dev] [PATCH 2/2] event/sw: simplify credit scheme Gage Eads
2017-12-11 13:42   ` Van Haaren, Harry
2017-12-16  8:54     ` Jerin Jacob

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9184057F7FC11744A2107296B6B8EB1E2BB164EB@FMSMSX108.amr.corp.intel.com \
    --to=gage.eads@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerin.jacob@caviumnetworks.com \
    --cc=nipun.gupta@nxp.com \
    --cc=santosh.shukla@caviumnetworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).