From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f51.google.com (mail-lf0-f51.google.com [209.85.215.51]) by dpdk.org (Postfix) with ESMTP id 6FB7C2C06 for ; Tue, 11 Oct 2016 10:52:44 +0200 (CEST) Received: by mail-lf0-f51.google.com with SMTP id l131so29276906lfl.2 for ; Tue, 11 Oct 2016 01:52:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=/cfoW8y/TcbmL3yMHdHs+Hl9gHvKHsJ7h6pXaJWr63Y=; b=oBFGzZHdqxmGu98C2kOF72Vp/AaiDta4808WjpgkBoFsU09PTK2icf/UqoJAoMQevY YRS5RIKcBiio1j+BMac1lKAUcpt4spQ8jeVMyQUwuDSyCbVcQTBV/D1mXspNP/g3FvGD UNDgj0h2I26ElxLK7S3wD5DsaIZLMgVfsujJOJIUkxP6Ow7T3pre1AUB++i+IU9Vc0Ge UNjTsJMPFEcUk7rYabE4cGkfTwsWTso+hBJ+aCsJo+cYLYTII/SvFuotgfNv0r1uw+Qg ehLFYeTgJhYEw7aq9TV+2O1ZzP9maBYgGp3vaGVve6dKPW2U1vgiN4fuOQi2aEG/Dar/ biwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=/cfoW8y/TcbmL3yMHdHs+Hl9gHvKHsJ7h6pXaJWr63Y=; b=ie6pGcIpT/8O+VOet2JsVpPYAoiPRsiWHmIdVXzNI6GtXeQ6IN12xJrDrT0u+FzZmt Y7RfFFtolXsoozCyWcICn6lqECZRmC9Z5BXSDXwmhva1h2gpF/iNRrzvsxqILRoyjGxB y3qIQmOIt27G2LnKexcANt5b70xkNJvCQ5sN0UhPCJ/rbNEqcrNJMMHKtzr0eZQEhmef kuqvZj/9aE09lMMzonu6mGPd16pSvaOnJsNMy/xajk2m3bqDVb1J54UMWjS108KEhHh+ i6oPZTqIghPPBIRX9EtD9uxGniMj0HVMHfuAO1R5K0xNnbiUHLkinNu6P+15/SbK2eu2 U3iA== X-Gm-Message-State: AA6/9Rm6oc5h+2jStmeeWHutWJvpzjGO7eRQAsoQxvxldV6p2eGtVRMACAVHurfX6/TzpxSx X-Received: by 10.25.160.130 with SMTP id j124mr1249844lfe.53.1476175960075; Tue, 11 Oct 2016 01:52:40 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id p132sm507833lfp.12.2016.10.11.01.52.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Oct 2016 01:52:39 -0700 (PDT) From: Thomas Monjalon To: "Kerlin, MarcinX" Cc: dev@dpdk.org, "De Lara Guarch, Pablo" Date: Tue, 11 Oct 2016 10:52:37 +0200 Message-ID: <10042732.sOty05EDuD@xps13> User-Agent: KMail/4.14.10 (Linux/4.5.4-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <68D830D942438745AD09BAFA99E33E812BE66F@IRSMSX102.ger.corp.intel.com> References: <1474974783-4861-2-git-send-email-marcinx.kerlin@intel.com> <4741418.sMAp9bqNYx@xps13> <68D830D942438745AD09BAFA99E33E812BE66F@IRSMSX102.ger.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against overwrite device data X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2016 08:52:44 -0000 2016-10-07 12:23, Kerlin, MarcinX: > Hi Thomas, > > > -----Original Message----- > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > Sent: Thursday, October 06, 2016 4:53 PM > > To: Kerlin, MarcinX > > Cc: dev@dpdk.org; De Lara Guarch, Pablo > > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite > > device data > > > > 2016-09-30 16:00, Marcin Kerlin: > > > Added protection against overwrite device data in array > > > rte_eth_dev_data[] for the next secondary applications. Secondary > > > process appends in the first free place rather than at the beginning. > > > This behavior prevents overwriting devices data of primary process by > > secondary process. > > > > It would be good to state what is a secondary process. > > You are trying to extend its capabilities to be able to initialize devices. > > So primary and secondary processes are almost equivalent? > > What happens if we do not create any device in the primary? > > Answer from code review: "Cannot allocate memzone for ethernet port data\n" > > > > The secondary process is a hack to me. > > But it is fine to have such hack for debug or monitoring purpose. > > I would like to understand what are the other use cases? > > It's true, it is fine for debug or monitoring but If DPDK allow run secondary app with > devices then it should be safe or completely not allowed. > > This bug has been observed while running secondary testpmd with virtual devices. > > I will adapt to the decision of maintainers regards to design of secondary process. > > > > > By the way, the code managing the shared data of a device should be at the > > EAL level in order to be used by other interfaces like crypto. > > > > > @@ -631,6 +692,8 @@ int > > > rte_eth_dev_detach(uint8_t port_id, char *name) { > > > struct rte_pci_addr addr; > > > + struct rte_eth_dev_data *eth_dev_data = NULL; > > > + char device[RTE_ETH_NAME_MAX_LEN]; > > > int ret = -1; > > > > > > if (name == NULL) { > > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name) > > > if (rte_eth_dev_is_detachable(port_id)) > > > goto err; > > > > > > + /* get device name by port id */ > > > + if (rte_eth_dev_get_name_by_port(port_id, device)) > > > + goto err; > > > + > > > + /* look for an entry in the shared device data */ > > > + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device); > > > + if (eth_dev_data == NULL) > > > + goto err; > > > > Why not getting eth_dev_data from rte_eth_devices[port_id].data ? > > because rte_eth_devices[port_id].data for some drivers (mainly virtual devices) > is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other drivers). > This causes that local eth_dev_data is clearing rather than shared in memzone. > > Naming is unique so if device was added then there (shared memzone) has to be. Not sure to understand. Isn't it a bug to have local eth_dev_data? It means these devices are not shared with secondary process? > > > --- a/lib/librte_ether/rte_ethdev.h > > > +++ b/lib/librte_ether/rte_ethdev.h > > > /** > > > * @internal > > > + * Release device data kept in shared memory for all processes. > > > + * > > > + * @param port_id > > > + * The port identifier of the device to release device data. > > > + * @return > > > + * - 0 on success, negative on error > > > + */ > > > +int rte_eth_dev_release_dev_data(uint8_t port_id); > > > > Why this function? It is not used. > > You already have done the job in the detach function. > > This function is using in testpmd.c:1640, basic wrapper for clean up. Please explain the need for cleaning on testpmd exit. Is it cleaning every devices even those used by the primary process? I feel it is very weak to not clearly define which process owns a device. > Detach function is working only for detachable devices, release_dev_data() > no matter just clean up shared array before next run secondary e.g testpmd. Yes freeing device resources is for detachable devices.