From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH] eventdev: Fix links_map initialization Date: Mon, 6 Mar 2017 22:21:54 +0530 Message-ID: <20170306165152.GA12775@localhost.localdomain> References: <1488430056-32055-1-git-send-email-gage.eads@intel.com> <20170306130625.GA4250@localhost.localdomain> <9184057F7FC11744A2107296B6B8EB1E01E6F9F5@FMSMSX108.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" , "Richardson, Bruce" , "hemant.agrawal@nxp.com" , "Van Haaren, Harry" , "nipun.gupta@nxp.com" To: "Eads, Gage" Return-path: Received: from NAM01-SN1-obe.outbound.protection.outlook.com (mail-sn1nam01on0050.outbound.protection.outlook.com [104.47.32.50]) by dpdk.org (Postfix) with ESMTP id 3E717FA70 for ; Mon, 6 Mar 2017 17:52:14 +0100 (CET) Content-Disposition: inline In-Reply-To: <9184057F7FC11744A2107296B6B8EB1E01E6F9F5@FMSMSX108.amr.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > Cc: dev@dpdk.org; Richardson, Bruce ; > > hemant.agrawal@nxp.com; Van Haaren, Harry ; > > 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 > > > --- > > > 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.