From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id AD7EA7D04 for ; Thu, 5 Oct 2017 10:13:04 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2017 01:13:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,480,1500966000"; d="scan'208";a="159112387" Received: from nikhilr-mobl.amr.corp.intel.com (HELO [10.106.152.123]) ([10.106.152.123]) by fmsmga005.fm.intel.com with ESMTP; 05 Oct 2017 01:13:00 -0700 To: Jerin Jacob Cc: bruce.richardson@intel.com, gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net, harry.van.haaren@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com, narender.vangati@intel.com, erik.g.carrillo@intel.com, abhinandan.gujjar@intel.com, santosh.shukla@caviumnetworks.com References: <1506028634-22998-1-git-send-email-nikhil.rao@intel.com> <1506028634-22998-4-git-send-email-nikhil.rao@intel.com> <20170922091032.GA5895@jerin> <20171003135248.GA10493@jerin> From: "Rao, Nikhil" Message-ID: <840c1f48-1d74-56bc-b295-4c20693f5217@intel.com> Date: Thu, 5 Oct 2017 13:42:59 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171003135248.GA10493@jerin> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter 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: Thu, 05 Oct 2017 08:13:05 -0000 On 10/3/2017 7:22 PM, Jerin Jacob wrote: > -----Original Message----- >> Date: Sun, 24 Sep 2017 23:46:51 +0530 >> From: "Rao, Nikhil" >> To: Jerin Jacob >> CC: bruce.richardson@intel.com, gage.eads@intel.com, dev@dpdk.org, >> thomas@monjalon.net, harry.van.haaren@intel.com, hemant.agrawal@nxp.com, >> nipun.gupta@nxp.com, narender.vangati@intel.com, >> erik.g.carrillo@intel.com, abhinandan.gujjar@intel.com, >> santosh.shukla@caviumnetworks.com >> Subject: Re: [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter >> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 >> Thunderbird/52.3.0 >> >> >> OK, Thanks for the detailed review. Will add the programmer guide to RC1. > > OK. Thanks. > >> >>> >>> >>> >> Yes, if create() and queue_add() are called from different processes, it >> wouldn't work. >> >>>> + >>>> +static uint8_t default_rss_key[] = { >>>> + 0x6d, 0x5a, 0x56, 0xda, 0x25, 0x5b, 0x0e, 0xc2, >>>> + 0x41, 0x67, 0x25, 0x3d, 0x43, 0xa3, 0x8f, 0xb0, >>>> + 0xd0, 0xca, 0x2b, 0xcb, 0xae, 0x7b, 0x30, 0xb4, >>>> + 0x77, 0xcb, 0x2d, 0xa3, 0x80, 0x30, 0xf2, 0x0c, >>>> + 0x6a, 0x42, 0xb7, 0x3b, 0xbe, 0xac, 0x01, 0xfa, >>>> +}; >>> >>> Looks like the scope of this array is only for rte_event_eth_rx_adapter_init, >>> if so please move it to stack. >>> >> OK. >> >>> >>>> +static uint8_t *rss_key_be; >>> >>> Can we remove this global variable add it in in adapter memory? >>> >> >> There is currently struct rte_event_eth_rx_adapter >> **rte_event_eth_rx_adapter that is an array of pointers to the adapters. >> rss_key_be points to memory after this array. >> >> are you thinking of something like: >> >> struct { >> struct rte_event_eth_rx_adapter **rte_event_eth_rx_adapter >> uint8_t *rss_key_be; >> } global; > > I was thinking, to hold 40B in struct rte_event_eth_rx_adapter for > rss_key_be and initialize per rx_adapter to avoid global variable > as fill_event_buffer() has access to rte_event_eth_rx_adapter. > > Something like below as rough idea. > ➜ [dpdk-next-eventdev] $ git diff > lib/librte_eventdev/rte_event_eth_rx_adapter.c > diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c > b/lib/librte_eventdev/rte_event_eth_rx_adapter.c > index cd19e7c28..ba6148931 100644 > --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c > +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c > @@ -37,6 +37,7 @@ struct rte_eth_event_enqueue_buffer { > }; > > struct rte_event_eth_rx_adapter { > + uint8_t rss_key[40]; > /* event device identifier */ > uint8_t eventdev_id; > /* per ethernet device structure */ OK. >>>> + >>>> +static int >>>> +default_conf_cb(uint8_t id, uint8_t dev_id, >>>> + struct rte_event_eth_rx_adapter_conf *conf, void *arg) >>>> +{ >>>> + >>>> + ret = rte_event_port_setup(dev_id, port_id, port_conf); >>>> + if (ret) { >>>> + RTE_EDEV_LOG_ERR("failed to setup event port %u\n", >>>> + port_id); >>> >>> return or add goto to exit from here to avoid calling rte_event_dev_start below >>> >> Could do the return but I wanted to leave the device in the same state as it >> was at entry into this function. Thoughts ? > > Will calling rte_event_dev_start() down(in case if wont return) change > the state? if not, it is fine. > OK, will put in the return. if the device were configured with an additional port and the setup for this port fails. The rte_event_dev_start() call will dereference a NULL ptr. Nikhil > No another comments. Looks good to me. > >