From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v10 20/20] ethdev: add control interface support Date: Thu, 20 Jul 2017 15:55:15 +0100 Message-ID: <092bbf4e-f448-7289-1df6-ec7c4cf747f7@intel.com> References: <20170704161337.45926-1-ferruh.yigit@intel.com> <20170704161337.45926-21-ferruh.yigit@intel.com> <20170708062859.GB11626@yliu-home> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, Stephen Hemminger , Bruce Richardson , Anatoly Burakov , Thomas Monjalon To: Yuanhan Liu Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 2F07929C8 for ; Thu, 20 Jul 2017 16:55:22 +0200 (CEST) In-Reply-To: <20170708062859.GB11626@yliu-home> Content-Language: en-US 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 7/8/2017 7:28 AM, Yuanhan Liu wrote: > On Tue, Jul 04, 2017 at 05:13:37PM +0100, Ferruh Yigit wrote: >> @@ -157,8 +164,12 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev, >> >> RTE_FUNC_PTR_OR_ERR_RET(*dev_init, -EINVAL); >> ret = dev_init(eth_dev); >> - if (ret) >> + if (ret) { >> rte_eth_dev_pci_release(eth_dev); >> + return ret; >> + } >> + >> + rte_eth_control_interface_create(eth_dev->data->port_id); > > Hi, > > So you are creating a virtual kernel interface for each PCI port. What > about the VDEVs? If you plan to create one for each port, why not create > it at the stage while allocating the eth device, or at the stage while > starting the port if the former is too earlier? Technically it is possible to support vdevs, but I don't know if there is usecase for it. If this is required, the change is simple, as you said this can be possible by moving create API to port start. > > Another thing comes to my mind is have you tried it with multi-process > model? Looks like it will create the control interface twice? Or it will > just be failed since the interface already exists? I didn't test mult-process scenarios, I will test. > > > I also have few questions regarding the whole design. So seems that the > ctrl_if only exports two APIs and they all will be only used in the EAL > layer. Thus, one question is did you plan to let APP use them? Judging > EAL already calls them automatically, I don't think it makes sense to > let the APP call it again. That being said, what's the point of the making > it be an lib? Why not just put it under EAL or somewhere else, and let > EAL invoke it as normal helper functions (instead of by public APIs)? Public APIs are from previous version of the patchset, where user application was in control on create/destroy and processing messages. With interfaces automatically created as you said these APIs are not very meaningful for application. But code is not so small to put into another library, I believe it is good to separate this code. > > I will avoid adding a new lib if possible. Otherwise, it increases the > chance of ABI/API breakage is needed in future for extensions. Those API are required for other libraries, not sure how to include the code otherwise. > > The same question goes to the ethtool lib. Since your solution can work > well with the well-known ethtool, which is also way more widely available > than the DPDK ethtool app, what's the point of keeping the ethtool app > then? Like above, I also don't think those APIs are meant for APPs (or > are they?). Thus, with the ethtool app removed, we then could again avoid > introducing a new lib. Ethtool library is ready to use abstraction on ethdev layer, I don't insist on having it as a separate library, but I believe it is good to reuse that code instead of re-writing it. > > --yliu >