From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API Date: Fri, 25 Aug 2017 15:26:36 +0200 Message-ID: <2468395.3cLPV6SE8H@xps> References: <5969739.W1oj9hpjs5@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org To: Shahaf Shuler , "Ananyev, Konstantin" Return-path: Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 7B3D77D14 for ; Fri, 25 Aug 2017 15:26:38 +0200 (CEST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 24/08/2017 09:12, Shahaf Shuler: > Thursday, August 24, 2017 1:06 AM, Thomas Monjalon: > > 23/08/2017 15:13, Shahaf Shuler: > > > Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin: > > > > From: Shahaf Shuler > > > > > In order to enable PMDs to support only one of the APIs, and > > > > > applications to avoid branching according to the underlying device > > > > > a copy functions to/from the old/new APIs were added. > > > > Looks a good intent. > > I would prefer the word "convert" instead of "copy". > > > > > > > int > > > > > rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id, > > [...] > > > > > + } else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) && > > > > > + (dev->data->dev_conf.rxmode.ignore == 1)) { > > > > > + int ret; > > > > > + struct rte_eth_rxmode rxmode; > > > > > + > > > > > + rte_eth_copy_rxq_offloads(&rxmode, rx_conf); > > > > > + if (memcmp(&rxmode, &dev->data->dev_conf.rxmode, > > > > > + sizeof(rxmode))) { > > > > > + /* > > > > > + * device which work with rxmode offloads API requires > > > > > + * a re-configuration in order to apply the new offloads > > > > > + * configuration. > > > > > + */ > > > > > + dev->data->dev_conf.rxmode = rxmode; > > > > > + ret = rte_eth_dev_configure(port_id, > > > > > + dev->data->nb_rx_queues, > > > > > + dev->data->nb_tx_queues, > > > > > + &dev->data->dev_conf); > > > > > > > > Hmm, and why we would need to reconfigure our device in the middle > > > > of rx queue setup? > > > > > > The reason is the old Rx offloads API is configured on device configure. > > > This if section is for applications which already moved to the new > > > offload API however the underlying PMD still uses the old one. > > > > Isn't it risky to re-run configure here? > > We could also declare this case as an error. > > > > I think applications which have migrated to the new API, could use the > > convert functions themselves before calling configure to support not > > migrated PMDs. > > The cons of my solution are: > > - discourage apps to migrate before all PMDs have migrated > > - expose a temporary function to convert API I propose it anyway because > > there is always someone to like bad ideas ;) > > Yes. I tried to make it as simple as possible for application to move to the new API. > Defining it as error flow, will enforce the application to check the PMD offload mode and branch accordingly. The conversion functions are a good helpers, yet the code remains complex due to the different cases with the different PMDs. > > Considering the re-configuration is risky, and without other ideas I will need to fall back to the error flow case. > Are we OK with that? I think we can take the risk of keeping this call to rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup(). In theory it should be acceptable. If we merge it soon, it can be better tested with every drivers.