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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, February 18, 2015 10:33 AM
> To: Iremonger, Bernard
> Cc: Tetsuya Mukawa; dev@dpdk.org; ivan.boule@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@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.
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.

Regards,

Bernard.