[dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs

[Tetsuya Mukawa]

On 2015/02/18 20:39, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
>> Sent: Wednesday, February 18, 2015 10:33 AM
>> To: Iremonger, Bernard
>> Cc: Tetsuya Mukawa; dev at dpdk.org; ivan.boule at 6wind.com
>> Subject: Re: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs
>>
>> Hi Bernard,
>>
>> 2015-02-18 10:26, Iremonger, Bernard:
>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tetsuya Mukawa
>>>> On 2015/02/18 10:02, Thomas Monjalon wrote:
>>>>> 2015-02-17 15:14, Tetsuya Mukawa:
>>>>>> On 2015/02/17 9:44, Thomas Monjalon wrote:
>>>>>>> 2015-02-16 13:14, Tetsuya Mukawa:
>>>>>>>> @@ -356,13 +342,24 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
>>>>>>>>  	}
>>>>>>>>  	else {
>>>>>>>>  		struct rte_pci_device *dev2 = NULL;
>>>>>>>> +		int ret;
>>>>>>>>
>>>>>>>>  		TAILQ_FOREACH(dev2, &pci_device_list, next) {
>>>>>>>> -			if (pci_addr_comparison(&dev->addr, &dev2->addr))
>>>>>>>> +			ret = eal_compare_pci_addr(&dev->addr, &dev2->addr);
>>>>>>>> +			if (ret > 0)
>>>>>>>>  				continue;
>>>>>>>> -			else {
>>>>>>>> +			else if (ret < 0) {
>>>>>>>>  				TAILQ_INSERT_BEFORE(dev2, dev, next);
>>>>>>>>  				return 0;
>>>>>>>> +			} else { /* already registered */
>>>>>>>> +				/* update pt_driver */
>>>>>>>> +				dev2->pt_driver = dev->pt_driver;
>>>>>>>> +				dev2->max_vfs = dev->max_vfs;
>>>>>>>> +				memmove(dev2->mem_resource,
>>>>>>>> +					dev->mem_resource,
>>>>>>>> +					sizeof(dev->mem_resource));
>>>>>>>> +				free(dev);
>>>>>>>> +				return 0;
>>>>>>> Could you comment this "else part" please?
>>>>>> PCI device list is allocated when rte_eal_init() is called. At
>>>>>> the time, to fill pci device information, sysfs value is used.
>>>>>> If sysfs values written by kernel device driver will not be
>>>>>> changed by igb_uio, vfio or pci_uio_genereic, above code isn't needed.
>>>>>> But actually above values are changed or added by them.
>>>>>>
>>>>>> Here is a step to cause issue.
>>>>>> 1. Boot linux.
>>>>>> 2. Start DPDK application without any physical NIC ports.
>>>>>>  - Here, some sysfs values are read, and store to pci device list.
>>>>>> 3. igb_uio starts managing a port.
>>>>>>  - Here, some sysfs values are changed.
>>>>>> 4. Add a NIC port to DPDK application using hotplug functions.
>>>>>>  - Here, we need to replace some values.
>>>>> I think that you are showing that something is wrongly designed in
>>>>> these EAL structures. I suggest to try cleaning this mess instead of workarounding.
>>> Hi Tetsuya, Thomas,
>>> I think that redesigning the EAL structures is beyond the scope of this patchset and should be
>> undertaken as a separate task.
>>
>> I strongly disagrees this opinion. We should never workaround design problems and add more
>> complex/weird code.
>> I think that this kind of consideration is the heart of some design problems we have to face today.
>> Please let's stop adding some code which just works without thinking the whole design.
>>
>>> I suspect there may be a problem in the original code when  a device which was using a kernel driver
>> is bound to igb_uio.  The igb_uio  driver adds /sys/bus/pci/devices/0000\:05\:00.0/max_vfs.
> Hi Tomas, Tetsuya,
>
> In general, I agree that we should not workaround design problems.
> In this case I don't think there is a problem with the rte_pci_device and pci_device_list structures.

I agree with it.

> The "already registered" device has been replaced. It would probably be cleaner to remove the "already registered" device from the list and then add the new device to the list rather than update the "already registered" device.
>

I guess "replacing" will not work, because rte_pci_device structure is
also registered in rte_eth_dev structure.
If we remove and free the pci device, I guess something goes wrong in
ethdev library.
Just removing is one more option, but it means there is a working pci
device that is not registered in the pci_device_list. I guess it's weird.

I still think updating may be correct behavior.
The pci_device_list is used like below when rte_eal_init() is called.

1. When rte_eal_pci_init() is called, all pci devices are registered in
the pci_device_list.
2. When rte_eal_dev_init() is called, dev_driver_list is traversed, and
if a PCI device for a driver is found in the pci_device_list, init() of
the driver is called.

I guess it's not so strange design.
But this design assumes pci_device_list is latest while matching a
driver registered in dev_driver_list.

 Now, we have hotplug patch.
I guess we should do same thing.
Before matching, we should update the pci_device_list.

Thanks,
Tetsuya

> Regards,
>
> Bernard.
>