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

Tetsuya Mukawa mukawa at igel.co.jp
Tue Feb 17 07:14:57 CET 2015


On 2015/02/17 9:44, Thomas Monjalon wrote:
> 2015-02-16 13:14, Tetsuya Mukawa:
>> This patch replaces pci_addr_comparison() and memcmp() of pci addresses by
>> eal_compare_pci_addr().
>>
>> v8:
>> - Fix pci_scan_one() to update sysfs values.
>>   (Thanks to Qiu, Michael and Iremonger, Bernard)
>> v5:
>> - Fix pci_scan_one to handle pt_driver correctly.
>> v4:
>> - Fix calculation method of eal_compare_pci_addr().
>> - Add parameter checking.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa at igel.co.jp>
>> ---
>>  lib/librte_eal/bsdapp/eal/eal_pci.c       | 29 ++++++++++++--------------
>>  lib/librte_eal/common/eal_common_pci.c    |  2 +-
>>  lib/librte_eal/common/include/rte_pci.h   | 34 +++++++++++++++++++++++++++++++
>>  lib/librte_eal/linuxapp/eal/eal_pci.c     | 30 +++++++++++++--------------
>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c |  2 +-
>>  5 files changed, 63 insertions(+), 34 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> index 74ecce7..7dbdccd 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> @@ -270,20 +270,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  	return (0);
>>  }
>>  
>> -/* Compare two PCI device addresses. */
>> -static int
>> -pci_addr_comparison(struct rte_pci_addr *addr, struct rte_pci_addr *addr2)
>> -{
>> -	uint64_t dev_addr = (addr->domain << 24) + (addr->bus << 16) + (addr->devid << 8) + addr->function;
>> -	uint64_t dev_addr2 = (addr2->domain << 24) + (addr2->bus << 16) + (addr2->devid << 8) + addr2->function;
>> -
>> -	if (dev_addr > dev_addr2)
>> -		return 1;
>> -	else
>> -		return 0;
>> -}
>> -
>> -
>>  /* Scan one pci sysfs entry, and fill the devices list from it. */
>>  static int
>>  pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
>> @@ -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.

> [...]
>> --- a/lib/librte_eal/common/include/rte_pci.h
>> +++ b/lib/librte_eal/common/include/rte_pci.h
>> @@ -269,6 +269,40 @@ eal_parse_pci_DomBDF(const char *input, struct rte_pci_addr *dev_addr)
>>  }
>>  #undef GET_PCIADDR_FIELD
>>  
>> +/* Compare two PCI device addresses. */
>> +/**
>> + * Utility function to compare two PCI device addresses.
>> + *
>> + * @param addr
>> + *	The PCI Bus-Device-Function address to compare
>> + * @param addr2
>> + *	The PCI Bus-Device-Function address to compare
>> + * @return
>> + *	0 on equal PCI address.
>> + *	Positive on addr is greater than addr2.
>> + *	Negative on addr is less than addr2, or error.
>> + */
>> +static inline int
>> +eal_compare_pci_addr(struct rte_pci_addr *addr, struct rte_pci_addr *addr2)
> I think that this function should be prefixed with rte_

Sure, I will.

> [...]
>>  		/* skip this element if it doesn't match our PCI address */
>> -		if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
>> +		if (eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr))
> Why memcmp is not sufficient to compare PCI addresses?
> The only exception I see is endianness for natural sorting.

Here is the definition.

struct rte_pci_addr {
        uint16_t domain;                /**< Device domain */
        uint8_t bus;                    /**< Device bus */
        uint8_t devid;                  /**< Device ID */
        uint8_t function;               /**< Device function. */
};

But, sizeof(struct rte_pci_addr) will be 6.
If rte_pci_addr is allocated in a function without bzero, last 1 byte
may have some value.
As a result, memcmp may not work. To avoid such a case, I checked like
above.

>>  			continue;
>>  
>>  		for (i = 0; i != uio_res->nb_maps; i++) {
>>




More information about the dev mailing list