From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id BEC657CB1 for ; Mon, 10 Jul 2017 08:14:15 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2017 23:14:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,338,1496127600"; d="scan'208";a="1170589383" Received: from nikhilr-mobl.amr.corp.intel.com (HELO [10.106.152.35]) ([10.106.152.35]) by fmsmga001.fm.intel.com with ESMTP; 09 Jul 2017 23:14:11 -0700 To: Jerin Jacob Cc: gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net, bruce.richardson@intel.com, harry.van.haaren@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com, narender.vangati@intel.com, Abhinandan Gujjar References: <29140c16-909a-1b9a-7391-481f900bd13c@intel.com> <1499377952-5306-1-git-send-email-nikhil.rao@intel.com> <20170706141829.GA5260@jerin> <02aef899-da84-9281-e4a4-2871237ea20e@intel.com> <20170707150317.GA2007@jerin> <20170707155707.GA6245@jerin> From: "Rao, Nikhil" Message-ID: <3d2d78cc-9572-bf95-6d25-9b350da62827@intel.com> Date: Mon, 10 Jul 2017 11:44:10 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170707155707.GA6245@jerin> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues 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: Mon, 10 Jul 2017 06:14:16 -0000 Hi Jerin, thanks for your feedback, further comments below. On 7/7/2017 9:27 PM, Jerin Jacob wrote: > -----Original Message----- >> Date: Fri, 7 Jul 2017 20:33:19 +0530 >> From: Jerin Jacob >>>> /* adapter has inbuilt port, no need to create producer port */ >>>> #define RTE_EVENT_ETHDEV_CAP_INBUILT_PORT (1ULL << 0) >>>> /* adapter does not need service function */ >>>> #define RTE_EVENT_ETHDEV_CAP_NO_SERVICE_FUNC (1ULL << 1) >> >> I just provided a name to share the view. You can choose better name. >> OK. >>>> int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t >>> eth_port_id); >>> I also think that the application should be able to call create() with > 1 ports. This would allow a single service to poll multiple NICs with WRR priority. The side effect of is that the queue add/del APIs would need to specify the port_id as well. >>>> int rte_event_eth_rx_adapter_get_info(uint8_t id, struct >>> rte_event_eth_rx_adap_info *info); >>>> int rte_event_eth_rx_adapter_configure(uint8_t id, struct >>> rte_event_eth_rx_adap_config *cfg); >>>> int rte_event_eth_rx_adapter_queue_add(uint8_t id, int32_t rx_queue_id, >>> const struct rte_eth_rx_event_adapter_queue_config *config); >>>> int rte_event_eth_rx_adapter_queue_del(uint8_t id, int32_t rx_queue_id) >>>> int rte_event_eth_rx_adapter_run(); The adapter's run function is not part of the public interface, agree ? >>>> int rte_event_eth_rx_adapter_free(uint8_t id); >>>> >>> >>> If I understood your idea, the function pointer struct would look something >>> like this. >>> >>> struct rte_eventdev_rx_adapter_ops { >>> rx_adapter_create_t create, >>> rx_adapter_get_info_t info, >>> rx_adapter_configure_t configure, >>> rx_adapter_queue_add_t queue_add, >>> rx_adapter_queue_del_t queue_del, >>> rx_adapter_queue_free_t queue_free, >>> rx_adapter_free_t free >>> }; >> >> Yes. But, I think, adapter create and free goes in struct rte_eventdev_op . >> See below. >> >> >>> >>> struct rte_eventdev { >>> .. >>> const struct rte_eventdev_rx_adapter_ops *rx_adapter_ops; >>> .. >>> }; >> >> An eventdev instance can have N adapters not just one. So this will be >> pointer to pointer, indexed through adapter_id. >> >> In SW PMD case, the eventdev may have only one adapter. >> In HW PMD cases, There will more than one. example, >> - if octeontx eventdev dev_id + any external PCI n/w card like i40e or >> nicvf case an adapter with similar adapter ops as SW PMD >> - if octeontx eventdev dev_id + octeontx ethdev dev_id case another >> adapter that does not need service cores to inject event to octeontx >> eventdev >> >> - Your generic Rx adapter we will make it as common code so that both SW PMD and >> octeontx PMD in service core mode as use the functions and register the >> ops. OK. >> >> >> I was thinking like this >> >> int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t eth_port_id) >> { >> eventdev_ops = ... /* access through dev_id */ >> >> /* common code to get ops memory from adapter id */ >> struct rte_eventdev_rx_adapter_ops* ops = rte_event_pmd_adapter_allocate(id); >> >> /* Fill in adapter ops from driver */ >> eventdev_ops->create_adapter(ops, dev_id, eth_port_id) As an implementation detail, the function pointer setup needs to account for DPDK primary/secondary processes, i.e, function addresses could be different in the 2 processes. >> >> } >> >> int rte_event_eth_rx_adapter_get_info(uint8_t id, struct rte_event_eth_rx_adap_info *info) >> { Argument list is missing the dev_id (as are the configure/queue_add/queue_del) functions. >> eventdev_ops = ... /* access through dev_id */ >> >> struct rte_eventdev_rx_adapter_ops* ops = eventdev_ops->rx_adapter_ops[id]; >> >> /* adapter specific info ops) >> ops->info(ops,....); >> >> } > > IMO, the mapping with your current proposal may go like this. > > - rte_event_eth_rx_adapter_create() // Will just fill the adapter ops from driver, probably > your existing rte_event_eth_rx_adapter_init() can go here > - exiting rte_event_eth_rx_adapter_create() will be more of > proposed rte_event_eth_rx_adapter_configure() on the given adapter. > - existing rte_event_eth_rx_adapter_queue_add() maps 1:1. But you can > remove eth_dev_id as it is provided proposed rte_event_eth_rx_adapter_create() See above for the case where the application could call create() with 2 different ports. > - same applies to rte_event_eth_rx_adapter_queue_del() > - exiting rte_event_eth_rx_adapter_stats_get() can be changed to > "xstat"(more like eventdev xstat scheme) so based on the adapter > capability application can get stats) > > > flow shall be: > 1) application calls rte_event_eth_rx_adapter_create(id, eventdev_id, > ethdev_id) to create the adapter.(Drivers fills the adapter ops based on > eventdev_id + ethdev_id capabilities) > 2) application calls rte_event_eth_rx_adapter_info_get() to get the > capability and other information of the adapter. > 3) application calls rte_event_eth_rx_adapter_configure() to configure > based on the application need and the adapter capability. > 4) application calls rte_event_eth_rx_adapter_queue_add() to link ethdev queues to > eventdev queues. On this call, The driver creates the service functions or programs the HW registers > based on the adapter capability to take the events from ethdev and inject to eventdev. > The flow looks good to me. >> >> >>> >>> But from previous emails (see struct rte_event_dev_producer_conf below), it >>> appears that the rx queue id and event information would be needed to create >>> the adapter that enqueues from the ethdev queue id to the event pmd, however >>> that information is available only at queue add time - thoughts ? >> >> Just eventdev_id and ethdev_id is enough to create adapter. OK.