[dpdk-dev] [PATCH v10 20/20] ethdev: add control interface support

[Ferruh Yigit]

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
>