Re: [dpdk-dev] [PATCH v10 13/14] eal/pci: Add rte_eal_dev_attach/detach() functions

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/02/21 21:49, Maxime Leroy wrote:
> Hi Tetsuya,
>
> On Sat, Feb 21, 2015 at 4:49 AM, Tetsuya Mukawa <mukawa@igel.co.jp> wrote:
>> On 2015/02/21 0:20, Maxime Leroy wrote:
> [...]
>>> Why you want to add devargs in the devargs_list, if there are no needs
>>> to store this information ?
>> In eal initialization code, virtual device names stored in devargs are
>> checked not to register a same device name twice.
>> And each init function of PMD just trust a device name received by eal.
>> So there is no code in PMD to check whether device name is unique.
>>
> I disagree with you. This check is not present in the master branch.
>
> You have added this check in your hotplug patchset, in this patch:
> [PATCH v10 10/14] eal/pci: Add a function to remove the entry of
> devargs list
> See: http://dpdk.org/ml/archives/dev/2015-February/013712.html
>
> Thus the problem should be already exist without your patches in the
> master branch.
>
> For example according to you, this testpmd command should create 2
> devices with the same name:
> testpmd -c 0xc --vdev eth_pcap0,iface=eth0 --vdev eth_pcap0,iface=eth1
> -n 2 -- -i
>
> But it's not the case:
> PMD: Initializing pmd_pcap for eth_pcap0
> PMD: Creating pcap-backed ethdev on numa socket 0
> PMD: Initializing pmd_pcap for eth_pcap0
> PMD: Creating pcap-backed ethdev on numa socket 0
> PMD: rte_eth_dev_allocate: Ethernet Device with name eth_pcap0 already
> allocated!
>
> In fact, it's not possible for any PMD_VDEV in the dpdk repo to create
> 2 devices with the same name.
> All the virtual device initialization functions use the
> rte_eth_dev_allocate function. This function prevents to create two
> ethernet devices with the same name:
>
>      if (rte_eth_dev_allocated(name) != NULL) {
>         PMD_DEBUG_TRACE("Ethernet Device with name %s already
> allocated!\n", name);
>         return NULL;
>     }
>

Ah, You are right.

>> For example, according to your suggestion, how to prevent below case?
>> $ ./testpmd -c f -n 1 -- -i
>> testpmd> port attach eth_pcap0,iface=eth0
>> testpmd> port attach eth_pcap0,iface=eth1
>>
>> Also, type below, after doing above.
>> testpmd> port detach 0
>>
>> Probably port 0 will be "eth_pcap0,iface=eth0".
>> But uninit code of PMD only receives a device name like 'eth_pcap0'.
>> (We have 2 'eth_pcap0' devices in PMD.)
>>
>> To prevent above case, probably we have 2 options at least.
>> One is changing init code of all virtual PMDs not to register same
>> device name.
> There are no need to change init code of all virtual PMDs to not
> register the same device name 2 times.
> Because it's already not possible to create 2 virtual device with the
> same name. (see my point above)
>
>> The other is to use devargs_list in EAL, and call init code of PMD with
>> a unique device name.
> Thus there are no needs to use the devargs_list for that.
>
> [..]
>>> But you don't call rte_eal_devargs_add with RTE_DEVTYPE_WHILISTED_PCI
>>> in  rte_eal_dev_attach_pdev ?
>> Yes, I don't.
>> Hotplug functions should not change BLACKLIST and WHITELIST.
>> So not to touch the list is correct behavior.
> Yes the correct behaviour for Hotplug functions is to not use the
> devargs_list for physical and virtual devices !

I totally agree with you now.
It seems I have a missunderstanding about code not to duplicate a vdev name.
Thanks for your suggestions, I will fix it in next patches.

Regards,
Tetsuya


>
> Regards,
>
> Maxime