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

[Iremonger, Bernard]

> >>>>>>>>>  /* Scan one pci sysfs entry, and fill the devices list from
> >>>>>>>>> it. */ static int  pci_scan_one(const char *dirname, uint16_t
> >>>>>>>>> domain, uint8_t bus, @@ -353,13 +339,20 @@ pci_scan_one(const
> >>>>>>>>> char *dirname, uint16_t domain, uint8_t bus,
> >>>>>>>>>  	}
> >>>>>>>>>  	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;
> >>>>>>> Hi Tetsuya,
> >>>>>>>
> >>>>>>> I am seeing a problem with the librte_pmd_ixgbe code where dev->max_vfs is being lost in
> some scenarios.
> >>>>>>> The following line should be added here:
> >>>>>>>       dev2->max_vfs = dev->max_vfs;
> >>>>>>>
> >>>>>>> numa_mode should probably be updated too (although it is not causing a problem at
> present).
> >>>>>>>       dev2->numa_mode = dev->numa_mode;

Hi Tetsuya, Michael,

The "already registered path" in the code is being executed.

dev->numa_mode does not change so the above line is not needed.

dev2->max_vfs has a value of 0  and dev->max_vfs has a value  of 2 ( 2 VF's have been configured previously).

dev2->mem_resource is different from dev->mem_resource so the following line needs to be added:

memmove(dev2->mem_resource, dev->mem_resource, sizeof(dev->mem_resource));

More replies below.
 
> >>>>>> I'm very curious, why those field miss? I haven't see any places
> >>>>>> clear this field.
> >>>>>>
> >>>>>> What is the root cause?

The max_vfs entry is created when igb_uio binds the device.
dev2  is the device already in the pci_device_list  so dev2->max_vfs  is 0.
dev is the new device being parsed, in this case dev->max_vfs is 2.
So the dev2->max_vfs  value needs to be updated.
Similarly  dev2-> pt_driver needs to be updated as its value is 0 (this is already done).
Similarly dev2->mem_resource needs to be updated as it is different from dev->mem_resource. 


> >>>>> Hi Michael,
> >>>>>
> >>>>> Here is my guess.
> >>>>> The above function creates pci device list.
> >>>> I am sorry. I forgot to add below information.
> >>>>
> >>>> "max_vfs" or "numa_node" value is came from sysfs when the above
> >>>> function is processed.
> >>> Yes, but it has already been registered, why it missed?
> >> Yes, it has been registered already, but probably should be updated.
> >> I guess sysfs value will be changed when igb_uio starts managing the device.
> >>
> >> ex)
> >> 1. Boot linux
> >> 2. start a dpdk application with no port.
> >> 3. pci device list is registered.
> >>  - Here, "max_vfs" is came from sysfs. Or there is no such a entry.
> >> 4. igb_uio binds the device.
> >> 5.  I guess max_vfs value of sysfs is changed. Or max_vfs entry is created.
> >> 6. The dpdk application calls hotplug function.
> > Yes, agree.
> >
> > But numa node can be changed?
> 
> Hi Michael,
> 
> I may misunderstand meaning of numa_node.
> I thought it indicated which numa node was nearest from the pci device, so it could not be
> configurable.
> BTW, I will be out of office tomorrow. So, I will submit v8 patches next Monday.
> 
> Thanks,
> Tetsuya
> 
> >
> > Bernard, does your issue occur after max_vfs changed in igb_uio?
> >
> > If not, I think must be figure out the reason.
> >
> > Thanks,
> > Michael
> >>  - Here, I guess we need to update "max_vfs" value.
> >>
> >> Above is a just my assumption.
> >> It may be good to wait for Bernard's reply.
> >>
> >> Thanks,
> >> Tetsuya
> >>
> >>> Thanks,
> >>> Michael
> >>>>> And current DPDK implementation assumes all devices needed to be
> >>>>> managed are under igb_uio or vfio when above code is processed.
> >>>>> To add hotplug function, we also need to think some devices will
> >>>>> start to be managed under igb_uio or vfio after initializing pci device list.
> >>>>> Anyway, I guess "max_vfs" value will be changed when igb_uio or
> >>>>> vfio manages the device.
> >>>>>
> >>>>> Hi Bernard,
> >>>>>
> >>>>> Could you please check "max_vfs" and "num_node" values, then check
> >>>>> the values again after the device is managed by igb_uio or vfio?
> >>>>> In my environment, it seems max_vfs is created by igb_uio.

Yes, max_vfs is created by igb_uio

> >>>>> But my NIC doesn't have VF, so behavior might be different in your
> >>>>> environment.
> >>>>> I guess "numa_node" should not be changed theoretically.
> >>>>>
> >>>>> If my guess is correct, how about replacing following values?
> >>>>> - driver

Agreed 

> >>>>> - max_vfs

Agreed

> >>>>> - resource

Agreed

> >>>>> - (numa_node)

Not necessary, as it does not change.


> >>>>> Except for above value, I guess other value shouldn't be changed
> >>>>> even after the device is managed by igb_uio or vfio.
> >>>>>
> >>>>> Thanks,
> >>>>> Tetsuya
> >>>>>
> >>>>>> Thanks,
> >>>>>> Michael
> >>>>>>

The scenario I am using is as follows:

Terminal1: rmmod ixgbe
Terminal1: rmmod ixgbevf
Terminal1:  rmmod igb_uio 

Terminal2: ./testpmd -c f -n 1 -d ../../librte_pmd_null.so -- -i --no-flush-rx

Terminal1:  insmod ./igb_uio.ko

Terminal3: ./dpdk_nic_bind.py -b igb_uio 0000:05:00.0

Terminal2: testpmd> port attach 0000:05:00.0

Regards,

Bernard.