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
>
