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 2F07929C8 for ; Thu, 20 Jul 2017 16:55:22 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Jul 2017 07:55:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,384,1496127600"; d="scan'208";a="110102123" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.249.70.107]) ([10.249.70.107]) by orsmga004.jf.intel.com with ESMTP; 20 Jul 2017 07:55:17 -0700 To: Yuanhan Liu Cc: dev@dpdk.org, Stephen Hemminger , Bruce Richardson , Anatoly Burakov , Thomas Monjalon References: <20170704161337.45926-1-ferruh.yigit@intel.com> <20170704161337.45926-21-ferruh.yigit@intel.com> <20170708062859.GB11626@yliu-home> From: Ferruh Yigit Message-ID: <092bbf4e-f448-7289-1df6-ec7c4cf747f7@intel.com> Date: Thu, 20 Jul 2017 15:55:15 +0100 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: <20170708062859.GB11626@yliu-home> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v10 20/20] ethdev: add control interface support 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, 20 Jul 2017 14:55:23 -0000 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 >