diff mbox

[dpdk-dev] eventdev: Fix links_map initialization

Message ID 1488430056-32055-1-git-send-email-gage.eads@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Eads, Gage March 2, 2017, 4:47 a.m. UTC 
  This patch initializes the links_map array entries to
EVENT_QUEUE_SERVICE_PRIORITY_INVALID, as expected by
rte_event_port_links_get().

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 lib/librte_eventdev/rte_eventdev.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Jerin Jacob March 6, 2017, 1:06 p.m. UTC | #1 
On Wed, Mar 01, 2017 at 10:47:36PM -0600, Gage Eads wrote:
> This patch initializes the links_map array entries to
> EVENT_QUEUE_SERVICE_PRIORITY_INVALID, as expected by
> rte_event_port_links_get().
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
>  lib/librte_eventdev/rte_eventdev.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
> index 68bfc3b..b8cd92b 100644
> --- a/lib/librte_eventdev/rte_eventdev.c
> +++ b/lib/librte_eventdev/rte_eventdev.c
> @@ -190,6 +190,8 @@ rte_event_dev_queue_config(struct rte_eventdev *dev, uint8_t nb_queues)
>  	return 0;
>  }
>  
> +#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
> +
>  static inline int
>  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
>  {
> @@ -251,6 +253,9 @@ rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
>  					"nb_ports %u", nb_ports);
>  			return -(ENOMEM);
>  		}
> +		for (i = 0; i < nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV; i++)
> +			dev->data->links_map[i] =
> +				EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
>  	} else if (dev->data->ports != NULL && nb_ports != 0) {/* re-config */
>  		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->port_release, -ENOTSUP);
>  
> @@ -305,6 +310,10 @@ rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
>  
>  		if (nb_ports > old_nb_ports) {
>  			uint8_t new_ps = nb_ports - old_nb_ports;
> +			unsigned int old_links_map_end =
> +				old_nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
> +			unsigned int links_map_end =
> +				nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
>  
>  			memset(ports + old_nb_ports, 0,
>  				sizeof(ports[0]) * new_ps);
> @@ -312,9 +321,9 @@ rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
>  				sizeof(ports_dequeue_depth[0]) * new_ps);
>  			memset(ports_enqueue_depth + old_nb_ports, 0,
>  				sizeof(ports_enqueue_depth[0]) * new_ps);
> -			memset(links_map +
> -				(old_nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV),
> -				0, sizeof(ports_enqueue_depth[0]) * new_ps);
> +			for (i = old_links_map_end; i < links_map_end; i++)
> +				links_map[i] =
> +					EVENT_QUEUE_SERVICE_PRIORITY_INVALID;

rte_event_port_setup() has rte_event_port_unlink() at the end of the
function. On rte_event_port_unlink, we are doing the same
operation(writing EVENT_QUEUE_SERVICE_PRIORITY_INVALID) and
rte_event_port_links_get() should be called after rte_event_dev_start(),
If so, Do you still think this duplicates writes are required? or Do you
have any other call sequence in mind?

---------------------------- Description from header file-----------------
The functions exported by the application Event API to setup a device
designated by its device identifier must be invoked in the following order:
    - rte_event_dev_configure()
    - rte_event_queue_setup()
    - rte_event_port_setup()
    - rte_event_port_link()
    - rte_event_dev_start()

Then, the application can invoke, in any order, the functions
exported by the Event API to schedule events, dequeue events, enqueue events,
change event queue(s) to event port [un]link establishment and so on.

Application may use rte_event_[queue/port]_default_conf_get() to get the
default configuration to set up an event queue or event port by
overriding few default values.

If the application wants to change the configuration (i.e. call
rte_event_dev_configure(), rte_event_queue_setup(), or
rte_event_port_setup()), it must call rte_event_dev_stop() first to stop the
device and then do the reconfiguration before calling rte_event_dev_start()
again. The schedule, enqueue and dequeue functions should not be invoked
when the device is stopped.
-----------------------------------------------------------------------------



>  		}
>  
>  		dev->data->ports = ports;
> @@ -815,8 +824,6 @@ rte_event_port_link(uint8_t dev_id, uint8_t port_id,
>  	return diag;
>  }
>  
> -#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
> -
>  int
>  rte_event_port_unlink(uint8_t dev_id, uint8_t port_id,
>  		      uint8_t queues[], uint16_t nb_unlinks)
> -- 
> 2.7.4
>
Eads, Gage March 6, 2017, 2:38 p.m. UTC | #2 
>  -----Original Message-----
>  From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>  Sent: Monday, March 6, 2017 7:06 AM
>  To: Eads, Gage <gage.eads@intel.com>
>  Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
>  hemant.agrawal@nxp.com; Van Haaren, Harry <harry.van.haaren@intel.com>;
>  nipun.gupta@nxp.com
>  Subject: Re: [PATCH] eventdev: Fix links_map initialization
>  
>  On Wed, Mar 01, 2017 at 10:47:36PM -0600, Gage Eads wrote:
>  > This patch initializes the links_map array entries to
>  > EVENT_QUEUE_SERVICE_PRIORITY_INVALID, as expected by
>  > rte_event_port_links_get().
>  >
>  > Signed-off-by: Gage Eads <gage.eads@intel.com>
>  > ---
>  >  lib/librte_eventdev/rte_eventdev.c | 17 ++++++++++++-----
>  >  1 file changed, 12 insertions(+), 5 deletions(-)
>  >
>  > diff --git a/lib/librte_eventdev/rte_eventdev.c
>  > b/lib/librte_eventdev/rte_eventdev.c
>  > index 68bfc3b..b8cd92b 100644
>  > --- a/lib/librte_eventdev/rte_eventdev.c
>  > +++ b/lib/librte_eventdev/rte_eventdev.c
>  > @@ -190,6 +190,8 @@ rte_event_dev_queue_config(struct rte_eventdev
>  *dev, uint8_t nb_queues)
>  >  	return 0;
>  >  }
>  >
>  > +#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
>  > +
>  >  static inline int
>  >  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
>  > { @@ -251,6 +253,9 @@ rte_event_dev_port_config(struct rte_eventdev
>  > *dev, uint8_t nb_ports)
>  >  					"nb_ports %u", nb_ports);
>  >  			return -(ENOMEM);
>  >  		}
>  > +		for (i = 0; i < nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
>  i++)
>  > +			dev->data->links_map[i] =
>  > +				EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
>  >  	} else if (dev->data->ports != NULL && nb_ports != 0) {/* re-config */
>  >  		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->port_release,
>  -ENOTSUP);
>  >
>  > @@ -305,6 +310,10 @@ rte_event_dev_port_config(struct rte_eventdev
>  > *dev, uint8_t nb_ports)
>  >
>  >  		if (nb_ports > old_nb_ports) {
>  >  			uint8_t new_ps = nb_ports - old_nb_ports;
>  > +			unsigned int old_links_map_end =
>  > +				old_nb_ports *
>  RTE_EVENT_MAX_QUEUES_PER_DEV;
>  > +			unsigned int links_map_end =
>  > +				nb_ports *
>  RTE_EVENT_MAX_QUEUES_PER_DEV;
>  >
>  >  			memset(ports + old_nb_ports, 0,
>  >  				sizeof(ports[0]) * new_ps);
>  > @@ -312,9 +321,9 @@ rte_event_dev_port_config(struct rte_eventdev
>  *dev, uint8_t nb_ports)
>  >  				sizeof(ports_dequeue_depth[0]) * new_ps);
>  >  			memset(ports_enqueue_depth + old_nb_ports, 0,
>  >  				sizeof(ports_enqueue_depth[0]) * new_ps);
>  > -			memset(links_map +
>  > -				(old_nb_ports *
>  RTE_EVENT_MAX_QUEUES_PER_DEV),
>  > -				0, sizeof(ports_enqueue_depth[0]) * new_ps);
>  > +			for (i = old_links_map_end; i < links_map_end; i++)
>  > +				links_map[i] =
>  > +
>  	EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
>  
>  rte_event_port_setup() has rte_event_port_unlink() at the end of the function.
>  On rte_event_port_unlink, we are doing the same operation(writing
>  EVENT_QUEUE_SERVICE_PRIORITY_INVALID) and
>  rte_event_port_links_get() should be called after rte_event_dev_start(), If so,
>  Do you still think this duplicates writes are required? or Do you have any other
>  call sequence in mind?

Ah, I didn't realize that was a purpose of calling port_unlink at the end of
port_setup. There is, however, an issue with initializing through the port unlink
when called by rte_event_port_setup(). The for-loop in rte_event_port_unlink() to
reset the links_map runs from 0 to diag, and diag is 0 when the port is being set up
since it has no queues to unlink at that time. (This is at least true of the sw PMD,
but would be the case for others, I imagine.)

Perhaps a simpler form of this patch is to copy that for-loop, with the bound being
dev->data->nb_queues, into rte_event_port_setup() after rte_event_port_unlink()
is called (if it is successful). What do you think?
Jerin Jacob March 6, 2017, 4:51 p.m. UTC | #3 
On Mon, Mar 06, 2017 at 02:38:05PM +0000, Eads, Gage wrote:
> >  -----Original Message-----
> >  From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> >  Sent: Monday, March 6, 2017 7:06 AM
> >  To: Eads, Gage <gage.eads@intel.com>
> >  Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> >  hemant.agrawal@nxp.com; Van Haaren, Harry <harry.van.haaren@intel.com>;
> >  nipun.gupta@nxp.com
> >  Subject: Re: [PATCH] eventdev: Fix links_map initialization
> >  
> >  On Wed, Mar 01, 2017 at 10:47:36PM -0600, Gage Eads wrote:
> >  > This patch initializes the links_map array entries to
> >  > EVENT_QUEUE_SERVICE_PRIORITY_INVALID, as expected by
> >  > rte_event_port_links_get().
> >  >
> >  > Signed-off-by: Gage Eads <gage.eads@intel.com>
> >  > ---
> >  >  lib/librte_eventdev/rte_eventdev.c | 17 ++++++++++++-----
> >  >  1 file changed, 12 insertions(+), 5 deletions(-)
> >  >
> >  > diff --git a/lib/librte_eventdev/rte_eventdev.c
> >  > b/lib/librte_eventdev/rte_eventdev.c
> >  > index 68bfc3b..b8cd92b 100644
> >  > --- a/lib/librte_eventdev/rte_eventdev.c
> >  > +++ b/lib/librte_eventdev/rte_eventdev.c
> >  > @@ -190,6 +190,8 @@ rte_event_dev_queue_config(struct rte_eventdev
> >  *dev, uint8_t nb_queues)
> >  >  	return 0;
> >  >  }
> >  >
> >  > +#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
> >  > +
> >  >  static inline int
> >  >  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
> >  > { @@ -251,6 +253,9 @@ rte_event_dev_port_config(struct rte_eventdev
> >  > *dev, uint8_t nb_ports)
> >  >  					"nb_ports %u", nb_ports);
> >  >  			return -(ENOMEM);
> >  >  		}
> >  > +		for (i = 0; i < nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
> >  i++)
> >  > +			dev->data->links_map[i] =
> >  > +				EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
> >  >  	} else if (dev->data->ports != NULL && nb_ports != 0) {/* re-config */
> >  >  		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->port_release,
> >  -ENOTSUP);
> >  >
> >  > @@ -305,6 +310,10 @@ rte_event_dev_port_config(struct rte_eventdev
> >  > *dev, uint8_t nb_ports)
> >  >
> >  >  		if (nb_ports > old_nb_ports) {
> >  >  			uint8_t new_ps = nb_ports - old_nb_ports;
> >  > +			unsigned int old_links_map_end =
> >  > +				old_nb_ports *
> >  RTE_EVENT_MAX_QUEUES_PER_DEV;
> >  > +			unsigned int links_map_end =
> >  > +				nb_ports *
> >  RTE_EVENT_MAX_QUEUES_PER_DEV;
> >  >
> >  >  			memset(ports + old_nb_ports, 0,
> >  >  				sizeof(ports[0]) * new_ps);
> >  > @@ -312,9 +321,9 @@ rte_event_dev_port_config(struct rte_eventdev
> >  *dev, uint8_t nb_ports)
> >  >  				sizeof(ports_dequeue_depth[0]) * new_ps);
> >  >  			memset(ports_enqueue_depth + old_nb_ports, 0,
> >  >  				sizeof(ports_enqueue_depth[0]) * new_ps);
> >  > -			memset(links_map +
> >  > -				(old_nb_ports *
> >  RTE_EVENT_MAX_QUEUES_PER_DEV),
> >  > -				0, sizeof(ports_enqueue_depth[0]) * new_ps);
> >  > +			for (i = old_links_map_end; i < links_map_end; i++)
> >  > +				links_map[i] =
> >  > +
> >  	EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
> >  
> >  rte_event_port_setup() has rte_event_port_unlink() at the end of the function.
> >  On rte_event_port_unlink, we are doing the same operation(writing
> >  EVENT_QUEUE_SERVICE_PRIORITY_INVALID) and
> >  rte_event_port_links_get() should be called after rte_event_dev_start(), If so,
> >  Do you still think this duplicates writes are required? or Do you have any other
> >  call sequence in mind?
> 
> Ah, I didn't realize that was a purpose of calling port_unlink at the end of
> port_setup. There is, however, an issue with initializing through the port unlink
> when called by rte_event_port_setup(). The for-loop in rte_event_port_unlink() to
> reset the links_map runs from 0 to diag, and diag is 0 when the port is being set up
> since it has no queues to unlink at that time. (This is at least true of the sw PMD,
> but would be the case for others, I imagine.)

I see. It was not the case for HW PMD. But if it is helping SW PMD case then we can use
your original patch.

> 
> Perhaps a simpler form of this patch is to copy that for-loop, with the bound being
> dev->data->nb_queues, into rte_event_port_setup() after rte_event_port_unlink()
> is called (if it is successful). What do you think?

I think your original patch is fine. IMO, you can change the commit
message to reflect the issue and send v2 based on your existing v1.
diff mbox

Patch

diff --git a/lib/librte_eventdev/rte_eventdev.c b/lib/librte_eventdev/rte_eventdev.c
index 68bfc3b..b8cd92b 100644
--- a/lib/librte_eventdev/rte_eventdev.c
+++ b/lib/librte_eventdev/rte_eventdev.c
@@ -190,6 +190,8 @@  rte_event_dev_queue_config(struct rte_eventdev *dev, uint8_t nb_queues)
 	return 0;
 }
 
+#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
+
 static inline int
 rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
 {
@@ -251,6 +253,9 @@  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
 					"nb_ports %u", nb_ports);
 			return -(ENOMEM);
 		}
+		for (i = 0; i < nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV; i++)
+			dev->data->links_map[i] =
+				EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
 	} else if (dev->data->ports != NULL && nb_ports != 0) {/* re-config */
 		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->port_release, -ENOTSUP);
 
@@ -305,6 +310,10 @@  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
 
 		if (nb_ports > old_nb_ports) {
 			uint8_t new_ps = nb_ports - old_nb_ports;
+			unsigned int old_links_map_end =
+				old_nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
+			unsigned int links_map_end =
+				nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV;
 
 			memset(ports + old_nb_ports, 0,
 				sizeof(ports[0]) * new_ps);
@@ -312,9 +321,9 @@  rte_event_dev_port_config(struct rte_eventdev *dev, uint8_t nb_ports)
 				sizeof(ports_dequeue_depth[0]) * new_ps);
 			memset(ports_enqueue_depth + old_nb_ports, 0,
 				sizeof(ports_enqueue_depth[0]) * new_ps);
-			memset(links_map +
-				(old_nb_ports * RTE_EVENT_MAX_QUEUES_PER_DEV),
-				0, sizeof(ports_enqueue_depth[0]) * new_ps);
+			for (i = old_links_map_end; i < links_map_end; i++)
+				links_map[i] =
+					EVENT_QUEUE_SERVICE_PRIORITY_INVALID;
 		}
 
 		dev->data->ports = ports;
@@ -815,8 +824,6 @@  rte_event_port_link(uint8_t dev_id, uint8_t port_id,
 	return diag;
 }
 
-#define EVENT_QUEUE_SERVICE_PRIORITY_INVALID (0xdead)
-
 int
 rte_event_port_unlink(uint8_t dev_id, uint8_t port_id,
 		      uint8_t queues[], uint16_t nb_unlinks)