Re: [dpdk-dev] [PATCH] eventdev: Fix links_map initialization

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

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.