Re: [PATCH v7 04/14] eal/pci: Consolidate pci address comparison APIs

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 /* 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.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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).

quoted

quoted

quoted

quoted

quoted

quoted

quoted

      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.
 

quoted

quoted

quoted

quoted

quoted

quoted

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. 

quoted

quoted

quoted

quoted

quoted

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

quoted

Bernard, does your issue occur after max_vfs changed in igb_uio?

If not, I think must be figure out the reason.

Thanks,
Michael

quoted

 - 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

quoted

Thanks,
Michael

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

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 

quoted

quoted

quoted

quoted

quoted

- max_vfs

Agreed

quoted

quoted

quoted

quoted

quoted

- resource

Agreed

quoted

quoted

quoted

quoted

quoted

- (numa_node)

Not necessary, as it does not change.

quoted

quoted

quoted

quoted

quoted

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

quoted

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.