diff mbox series

[v2,3/4] eal: enable WC during resources mapping

Message ID 1530191753-18689-4-git-send-email-rk@semihalf.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series support for write combining |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Rafal Kozik June 28, 2018, 1:15 p.m. UTC 
  Write combining (WC) increases NIC performance by making better
utilization of PCI bus, but cannot be used by all PMDs.

It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
drivers flags. For proper work also igb_uio driver must be loaded with
wc_activate set to 1.

When mapping PCI resources, firstly try to us WC.
If it is not supported it will fallback to normal mode.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 2 files changed, 31 insertions(+), 12 deletions(-)

Comments

Ferruh Yigit June 28, 2018, 2:50 p.m. UTC | #1 
On 6/28/2018 2:15 PM, Rafal Kozik wrote:
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus, but cannot be used by all PMDs.
> 
> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
> drivers flags. For proper work also igb_uio driver must be loaded with
> wc_activate set to 1.
> 
> When mapping PCI resources, firstly try to us WC.
> If it is not supported it will fallback to normal mode.
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index d423e4b..e3947c2 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -282,22 +282,19 @@ int
>  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  		struct mapped_pci_resource *uio_res, int map_idx)
>  {
> -	int fd;
> +	int fd = -1;
>  	char devname[PATH_MAX];
>  	void *mapaddr;
>  	struct rte_pci_addr *loc;
>  	struct pci_map *maps;
> +	int wc_activate = 0;
> +
> +	if (dev->driver != NULL)
> +		wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
>  
>  	loc = &dev->addr;
>  	maps = uio_res->maps;
>  
> -	/* update devname for mmap  */
> -	snprintf(devname, sizeof(devname),
> -			"%s/" PCI_PRI_FMT "/resource%d",
> -			rte_pci_get_sysfs_path(),
> -			loc->domain, loc->bus, loc->devid,
> -			loc->function, res_idx);
> -
>  	/* allocate memory to keep path */
>  	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>  	if (maps[map_idx].path == NULL) {
> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	/*
>  	 * open resource file, to mmap it
>  	 */
> -	fd = open(devname, O_RDWR);
> -	if (fd < 0) {
> -		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +	if (wc_activate) {
> +		/* update devname for mmap  */
> +		snprintf(devname, sizeof(devname),
> +			"%s/" PCI_PRI_FMT "/resource%d_wc",
> +			rte_pci_get_sysfs_path(),
> +			loc->domain, loc->bus, loc->devid,
> +			loc->function, res_idx);
> +
> +		fd = open(devname, O_RDWR);

What do you think adding an error log here? If opening resource$_wc fails and
fallback to resource# file, there won't be any way for user to know about it.

> +	}
> +
> +	if (!wc_activate || fd < 0) {
> +		snprintf(devname, sizeof(devname),
> +			"%s/" PCI_PRI_FMT "/resource%d",
> +			rte_pci_get_sysfs_path(),
> +			loc->domain, loc->bus, loc->devid,
> +			loc->function, res_idx);
> +
> +		/* then try to map resource file */
> +		fd = open(devname, O_RDWR);
> +		if (fd < 0) {
> +			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>  				devname, strerror(errno));
> -		goto error;
> +			goto error;
> +		}
>  	}
>  
>  	/* try mapping somewhere close to the end of hugepages */
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 458e6d0..828acc5 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -135,6 +135,8 @@ struct rte_pci_bus {
>  
>  /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
>  #define RTE_PCI_DRV_NEED_MAPPING 0x0001
> +/** Device needs PCI BAR mapping with enabled write combining (wc) */
> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
>  /** Device driver supports link state interrupt */
>  #define RTE_PCI_DRV_INTR_LSC	0x0008
>  /** Device driver supports device removal interrupt */
>
Rafal Kozik June 29, 2018, 8:58 a.m. UTC | #2 
2018-06-28 16:50 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> On 6/28/2018 2:15 PM, Rafal Kozik wrote:
>> Write combining (WC) increases NIC performance by making better
>> utilization of PCI bus, but cannot be used by all PMDs.
>>
>> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
>> drivers flags. For proper work also igb_uio driver must be loaded with
>> wc_activate set to 1.
>>
>> When mapping PCI resources, firstly try to us WC.
>> If it is not supported it will fallback to normal mode.
>>
>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
>> index d423e4b..e3947c2 100644
>> --- a/drivers/bus/pci/linux/pci_uio.c
>> +++ b/drivers/bus/pci/linux/pci_uio.c
>> @@ -282,22 +282,19 @@ int
>>  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>               struct mapped_pci_resource *uio_res, int map_idx)
>>  {
>> -     int fd;
>> +     int fd = -1;
>>       char devname[PATH_MAX];
>>       void *mapaddr;
>>       struct rte_pci_addr *loc;
>>       struct pci_map *maps;
>> +     int wc_activate = 0;
>> +
>> +     if (dev->driver != NULL)
>> +             wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
>>
>>       loc = &dev->addr;
>>       maps = uio_res->maps;
>>
>> -     /* update devname for mmap  */
>> -     snprintf(devname, sizeof(devname),
>> -                     "%s/" PCI_PRI_FMT "/resource%d",
>> -                     rte_pci_get_sysfs_path(),
>> -                     loc->domain, loc->bus, loc->devid,
>> -                     loc->function, res_idx);
>> -
>>       /* allocate memory to keep path */
>>       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>>       if (maps[map_idx].path == NULL) {
>> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>       /*
>>        * open resource file, to mmap it
>>        */
>> -     fd = open(devname, O_RDWR);
>> -     if (fd < 0) {
>> -             RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>> +     if (wc_activate) {
>> +             /* update devname for mmap  */
>> +             snprintf(devname, sizeof(devname),
>> +                     "%s/" PCI_PRI_FMT "/resource%d_wc",
>> +                     rte_pci_get_sysfs_path(),
>> +                     loc->domain, loc->bus, loc->devid,
>> +                     loc->function, res_idx);
>> +
>> +             fd = open(devname, O_RDWR);
>
> What do you think adding an error log here? If opening resource$_wc fails and
> fallback to resource# file, there won't be any way for user to know about it.
>

This error will be misleading for two reasons:
 * even if open return success, it could silently fall-back to non
prefetchable mode,
 * NICs could have multiple BARs, but not all support WC. I such case
fails will be desirable.

>> +     }
>> +
>> +     if (!wc_activate || fd < 0) {
>> +             snprintf(devname, sizeof(devname),
>> +                     "%s/" PCI_PRI_FMT "/resource%d",
>> +                     rte_pci_get_sysfs_path(),
>> +                     loc->domain, loc->bus, loc->devid,
>> +                     loc->function, res_idx);
>> +
>> +             /* then try to map resource file */
>> +             fd = open(devname, O_RDWR);
>> +             if (fd < 0) {
>> +                     RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>                               devname, strerror(errno));
>> -             goto error;
>> +                     goto error;
>> +             }
>>       }
>>
>>       /* try mapping somewhere close to the end of hugepages */
>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
>> index 458e6d0..828acc5 100644
>> --- a/drivers/bus/pci/rte_bus_pci.h
>> +++ b/drivers/bus/pci/rte_bus_pci.h
>> @@ -135,6 +135,8 @@ struct rte_pci_bus {
>>
>>  /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
>>  #define RTE_PCI_DRV_NEED_MAPPING 0x0001
>> +/** Device needs PCI BAR mapping with enabled write combining (wc) */
>> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
>>  /** Device driver supports link state interrupt */
>>  #define RTE_PCI_DRV_INTR_LSC 0x0008
>>  /** Device driver supports device removal interrupt */
>>
>
Ferruh Yigit June 29, 2018, 9:05 a.m. UTC | #3 
On 6/29/2018 9:58 AM, Rafał Kozik wrote:
> 2018-06-28 16:50 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>> On 6/28/2018 2:15 PM, Rafal Kozik wrote:
>>> Write combining (WC) increases NIC performance by making better
>>> utilization of PCI bus, but cannot be used by all PMDs.
>>>
>>> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
>>> drivers flags. For proper work also igb_uio driver must be loaded with
>>> wc_activate set to 1.
>>>
>>> When mapping PCI resources, firstly try to us WC.
>>> If it is not supported it will fallback to normal mode.
>>>
>>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>>  drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
>>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
>>> index d423e4b..e3947c2 100644
>>> --- a/drivers/bus/pci/linux/pci_uio.c
>>> +++ b/drivers/bus/pci/linux/pci_uio.c
>>> @@ -282,22 +282,19 @@ int
>>>  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>               struct mapped_pci_resource *uio_res, int map_idx)
>>>  {
>>> -     int fd;
>>> +     int fd = -1;
>>>       char devname[PATH_MAX];
>>>       void *mapaddr;
>>>       struct rte_pci_addr *loc;
>>>       struct pci_map *maps;
>>> +     int wc_activate = 0;
>>> +
>>> +     if (dev->driver != NULL)
>>> +             wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
>>>
>>>       loc = &dev->addr;
>>>       maps = uio_res->maps;
>>>
>>> -     /* update devname for mmap  */
>>> -     snprintf(devname, sizeof(devname),
>>> -                     "%s/" PCI_PRI_FMT "/resource%d",
>>> -                     rte_pci_get_sysfs_path(),
>>> -                     loc->domain, loc->bus, loc->devid,
>>> -                     loc->function, res_idx);
>>> -
>>>       /* allocate memory to keep path */
>>>       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>>>       if (maps[map_idx].path == NULL) {
>>> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>       /*
>>>        * open resource file, to mmap it
>>>        */
>>> -     fd = open(devname, O_RDWR);
>>> -     if (fd < 0) {
>>> -             RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>> +     if (wc_activate) {
>>> +             /* update devname for mmap  */
>>> +             snprintf(devname, sizeof(devname),
>>> +                     "%s/" PCI_PRI_FMT "/resource%d_wc",
>>> +                     rte_pci_get_sysfs_path(),
>>> +                     loc->domain, loc->bus, loc->devid,
>>> +                     loc->function, res_idx);
>>> +
>>> +             fd = open(devname, O_RDWR);
>>
>> What do you think adding an error log here? If opening resource$_wc fails and
>> fallback to resource# file, there won't be any way for user to know about it.
>>
> 
> This error will be misleading for two reasons:
>  * even if open return success, it could silently fall-back to non
> prefetchable mode,
>  * NICs could have multiple BARs, but not all support WC. I such case
> fails will be desirable.

OK, perhaps not error log to prevent mislead, but what do you think "info" level
log, to notify the user that write combined version in not used.

> 
>>> +     }
>>> +
>>> +     if (!wc_activate || fd < 0) {
>>> +             snprintf(devname, sizeof(devname),
>>> +                     "%s/" PCI_PRI_FMT "/resource%d",
>>> +                     rte_pci_get_sysfs_path(),
>>> +                     loc->domain, loc->bus, loc->devid,
>>> +                     loc->function, res_idx);
>>> +
>>> +             /* then try to map resource file */
>>> +             fd = open(devname, O_RDWR);
>>> +             if (fd < 0) {
>>> +                     RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>                               devname, strerror(errno));
>>> -             goto error;
>>> +                     goto error;
>>> +             }
>>>       }
>>>
>>>       /* try mapping somewhere close to the end of hugepages */
>>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
>>> index 458e6d0..828acc5 100644
>>> --- a/drivers/bus/pci/rte_bus_pci.h
>>> +++ b/drivers/bus/pci/rte_bus_pci.h
>>> @@ -135,6 +135,8 @@ struct rte_pci_bus {
>>>
>>>  /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
>>>  #define RTE_PCI_DRV_NEED_MAPPING 0x0001
>>> +/** Device needs PCI BAR mapping with enabled write combining (wc) */
>>> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
>>>  /** Device driver supports link state interrupt */
>>>  #define RTE_PCI_DRV_INTR_LSC 0x0008
>>>  /** Device driver supports device removal interrupt */
>>>
>>
Rafal Kozik June 29, 2018, 10:28 a.m. UTC | #4 
2018-06-29 11:05 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> On 6/29/2018 9:58 AM, Rafał Kozik wrote:
>> 2018-06-28 16:50 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>>> On 6/28/2018 2:15 PM, Rafal Kozik wrote:
>>>> Write combining (WC) increases NIC performance by making better
>>>> utilization of PCI bus, but cannot be used by all PMDs.
>>>>
>>>> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
>>>> drivers flags. For proper work also igb_uio driver must be loaded with
>>>> wc_activate set to 1.
>>>>
>>>> When mapping PCI resources, firstly try to us WC.
>>>> If it is not supported it will fallback to normal mode.
>>>>
>>>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>>  drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
>>>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
>>>> index d423e4b..e3947c2 100644
>>>> --- a/drivers/bus/pci/linux/pci_uio.c
>>>> +++ b/drivers/bus/pci/linux/pci_uio.c
>>>> @@ -282,22 +282,19 @@ int
>>>>  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>>               struct mapped_pci_resource *uio_res, int map_idx)
>>>>  {
>>>> -     int fd;
>>>> +     int fd = -1;
>>>>       char devname[PATH_MAX];
>>>>       void *mapaddr;
>>>>       struct rte_pci_addr *loc;
>>>>       struct pci_map *maps;
>>>> +     int wc_activate = 0;
>>>> +
>>>> +     if (dev->driver != NULL)
>>>> +             wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
>>>>
>>>>       loc = &dev->addr;
>>>>       maps = uio_res->maps;
>>>>
>>>> -     /* update devname for mmap  */
>>>> -     snprintf(devname, sizeof(devname),
>>>> -                     "%s/" PCI_PRI_FMT "/resource%d",
>>>> -                     rte_pci_get_sysfs_path(),
>>>> -                     loc->domain, loc->bus, loc->devid,
>>>> -                     loc->function, res_idx);
>>>> -
>>>>       /* allocate memory to keep path */
>>>>       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>>>>       if (maps[map_idx].path == NULL) {
>>>> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>>       /*
>>>>        * open resource file, to mmap it
>>>>        */
>>>> -     fd = open(devname, O_RDWR);
>>>> -     if (fd < 0) {
>>>> -             RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>> +     if (wc_activate) {
>>>> +             /* update devname for mmap  */
>>>> +             snprintf(devname, sizeof(devname),
>>>> +                     "%s/" PCI_PRI_FMT "/resource%d_wc",
>>>> +                     rte_pci_get_sysfs_path(),
>>>> +                     loc->domain, loc->bus, loc->devid,
>>>> +                     loc->function, res_idx);
>>>> +
>>>> +             fd = open(devname, O_RDWR);
>>>
>>> What do you think adding an error log here? If opening resource$_wc fails and
>>> fallback to resource# file, there won't be any way for user to know about it.
>>>
>>
>> This error will be misleading for two reasons:
>>  * even if open return success, it could silently fall-back to non
>> prefetchable mode,
>>  * NICs could have multiple BARs, but not all support WC. I such case
>> fails will be desirable.
>
> OK, perhaps not error log to prevent mislead, but what do you think "info" level
> log, to notify the user that write combined version in not used.
>

In new revision of patch set I add logging of device name.
For every resources it provides information if mapped with or without WC.

It looks like:
EAL: /sys/bus/pci/devices/0000:00:06.0/resource0 mapped
EAL: /sys/bus/pci/devices/0000:00:06.0/resource2_wc mapped
EAL: /sys/bus/pci/devices/0000:00:06.0/resource4 mapped

>>
>>>> +     }
>>>> +
>>>> +     if (!wc_activate || fd < 0) {
>>>> +             snprintf(devname, sizeof(devname),
>>>> +                     "%s/" PCI_PRI_FMT "/resource%d",
>>>> +                     rte_pci_get_sysfs_path(),
>>>> +                     loc->domain, loc->bus, loc->devid,
>>>> +                     loc->function, res_idx);
>>>> +
>>>> +             /* then try to map resource file */
>>>> +             fd = open(devname, O_RDWR);
>>>> +             if (fd < 0) {
>>>> +                     RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>>                               devname, strerror(errno));
>>>> -             goto error;
>>>> +                     goto error;
>>>> +             }
>>>>       }
>>>>
>>>>       /* try mapping somewhere close to the end of hugepages */
>>>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
>>>> index 458e6d0..828acc5 100644
>>>> --- a/drivers/bus/pci/rte_bus_pci.h
>>>> +++ b/drivers/bus/pci/rte_bus_pci.h
>>>> @@ -135,6 +135,8 @@ struct rte_pci_bus {
>>>>
>>>>  /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
>>>>  #define RTE_PCI_DRV_NEED_MAPPING 0x0001
>>>> +/** Device needs PCI BAR mapping with enabled write combining (wc) */
>>>> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
>>>>  /** Device driver supports link state interrupt */
>>>>  #define RTE_PCI_DRV_INTR_LSC 0x0008
>>>>  /** Device driver supports device removal interrupt */
>>>>
>>>
>
Ferruh Yigit June 29, 2018, 10:37 a.m. UTC | #5 
On 6/29/2018 11:28 AM, Rafał Kozik wrote:
> 2018-06-29 11:05 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>> On 6/29/2018 9:58 AM, Rafał Kozik wrote:
>>> 2018-06-28 16:50 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
>>>> On 6/28/2018 2:15 PM, Rafal Kozik wrote:
>>>>> Write combining (WC) increases NIC performance by making better
>>>>> utilization of PCI bus, but cannot be used by all PMDs.
>>>>>
>>>>> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in
>>>>> drivers flags. For proper work also igb_uio driver must be loaded with
>>>>> wc_activate set to 1.
>>>>>
>>>>> When mapping PCI resources, firstly try to us WC.
>>>>> If it is not supported it will fallback to normal mode.
>>>>>
>>>>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>>>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>>>> ---
>>>>>  drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
>>>>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>>>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
>>>>> index d423e4b..e3947c2 100644
>>>>> --- a/drivers/bus/pci/linux/pci_uio.c
>>>>> +++ b/drivers/bus/pci/linux/pci_uio.c
>>>>> @@ -282,22 +282,19 @@ int
>>>>>  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>>>               struct mapped_pci_resource *uio_res, int map_idx)
>>>>>  {
>>>>> -     int fd;
>>>>> +     int fd = -1;
>>>>>       char devname[PATH_MAX];
>>>>>       void *mapaddr;
>>>>>       struct rte_pci_addr *loc;
>>>>>       struct pci_map *maps;
>>>>> +     int wc_activate = 0;
>>>>> +
>>>>> +     if (dev->driver != NULL)
>>>>> +             wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
>>>>>
>>>>>       loc = &dev->addr;
>>>>>       maps = uio_res->maps;
>>>>>
>>>>> -     /* update devname for mmap  */
>>>>> -     snprintf(devname, sizeof(devname),
>>>>> -                     "%s/" PCI_PRI_FMT "/resource%d",
>>>>> -                     rte_pci_get_sysfs_path(),
>>>>> -                     loc->domain, loc->bus, loc->devid,
>>>>> -                     loc->function, res_idx);
>>>>> -
>>>>>       /* allocate memory to keep path */
>>>>>       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>>>>>       if (maps[map_idx].path == NULL) {
>>>>> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>>>>       /*
>>>>>        * open resource file, to mmap it
>>>>>        */
>>>>> -     fd = open(devname, O_RDWR);
>>>>> -     if (fd < 0) {
>>>>> -             RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>>> +     if (wc_activate) {
>>>>> +             /* update devname for mmap  */
>>>>> +             snprintf(devname, sizeof(devname),
>>>>> +                     "%s/" PCI_PRI_FMT "/resource%d_wc",
>>>>> +                     rte_pci_get_sysfs_path(),
>>>>> +                     loc->domain, loc->bus, loc->devid,
>>>>> +                     loc->function, res_idx);
>>>>> +
>>>>> +             fd = open(devname, O_RDWR);
>>>>
>>>> What do you think adding an error log here? If opening resource$_wc fails and
>>>> fallback to resource# file, there won't be any way for user to know about it.
>>>>
>>>
>>> This error will be misleading for two reasons:
>>>  * even if open return success, it could silently fall-back to non
>>> prefetchable mode,
>>>  * NICs could have multiple BARs, but not all support WC. I such case
>>> fails will be desirable.
>>
>> OK, perhaps not error log to prevent mislead, but what do you think "info" level
>> log, to notify the user that write combined version in not used.
>>
> 
> In new revision of patch set I add logging of device name.
> For every resources it provides information if mapped with or without WC.
> 
> It looks like:
> EAL: /sys/bus/pci/devices/0000:00:06.0/resource0 mapped
> EAL: /sys/bus/pci/devices/0000:00:06.0/resource2_wc mapped
> EAL: /sys/bus/pci/devices/0000:00:06.0/resource4 mapped

Isn't this too verbose now? This is info level, not debug and which means will
be visible many cases and mapping resource0 is most common way and it will be
keep repeated.

What I asked was, if _wc is requested and failed it will fallback to default
resource file and user won't know anything about it, add a log about fallback so
that user can know what actually happens is different from what requested.

> 
>>>
>>>>> +     }
>>>>> +
>>>>> +     if (!wc_activate || fd < 0) {
>>>>> +             snprintf(devname, sizeof(devname),
>>>>> +                     "%s/" PCI_PRI_FMT "/resource%d",
>>>>> +                     rte_pci_get_sysfs_path(),
>>>>> +                     loc->domain, loc->bus, loc->devid,
>>>>> +                     loc->function, res_idx);
>>>>> +
>>>>> +             /* then try to map resource file */
>>>>> +             fd = open(devname, O_RDWR);
>>>>> +             if (fd < 0) {
>>>>> +                     RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
>>>>>                               devname, strerror(errno));
>>>>> -             goto error;
>>>>> +                     goto error;
>>>>> +             }
>>>>>       }
>>>>>
>>>>>       /* try mapping somewhere close to the end of hugepages */
>>>>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
>>>>> index 458e6d0..828acc5 100644
>>>>> --- a/drivers/bus/pci/rte_bus_pci.h
>>>>> +++ b/drivers/bus/pci/rte_bus_pci.h
>>>>> @@ -135,6 +135,8 @@ struct rte_pci_bus {
>>>>>
>>>>>  /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
>>>>>  #define RTE_PCI_DRV_NEED_MAPPING 0x0001
>>>>> +/** Device needs PCI BAR mapping with enabled write combining (wc) */
>>>>> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
>>>>>  /** Device driver supports link state interrupt */
>>>>>  #define RTE_PCI_DRV_INTR_LSC 0x0008
>>>>>  /** Device driver supports device removal interrupt */
>>>>>
>>>>
>>
diff mbox series

Patch

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index d423e4b..e3947c2 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -282,22 +282,19 @@  int
 pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		struct mapped_pci_resource *uio_res, int map_idx)
 {
-	int fd;
+	int fd = -1;
 	char devname[PATH_MAX];
 	void *mapaddr;
 	struct rte_pci_addr *loc;
 	struct pci_map *maps;
+	int wc_activate = 0;
+
+	if (dev->driver != NULL)
+		wc_activate = dev->driver->drv_flags & RTE_PCI_DRV_WC_ACTIVATE;
 
 	loc = &dev->addr;
 	maps = uio_res->maps;
 
-	/* update devname for mmap  */
-	snprintf(devname, sizeof(devname),
-			"%s/" PCI_PRI_FMT "/resource%d",
-			rte_pci_get_sysfs_path(),
-			loc->domain, loc->bus, loc->devid,
-			loc->function, res_idx);
-
 	/* allocate memory to keep path */
 	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
 	if (maps[map_idx].path == NULL) {
@@ -309,11 +306,31 @@  pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	/*
 	 * open resource file, to mmap it
 	 */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+	if (wc_activate) {
+		/* update devname for mmap  */
+		snprintf(devname, sizeof(devname),
+			"%s/" PCI_PRI_FMT "/resource%d_wc",
+			rte_pci_get_sysfs_path(),
+			loc->domain, loc->bus, loc->devid,
+			loc->function, res_idx);
+
+		fd = open(devname, O_RDWR);
+	}
+
+	if (!wc_activate || fd < 0) {
+		snprintf(devname, sizeof(devname),
+			"%s/" PCI_PRI_FMT "/resource%d",
+			rte_pci_get_sysfs_path(),
+			loc->domain, loc->bus, loc->devid,
+			loc->function, res_idx);
+
+		/* then try to map resource file */
+		fd = open(devname, O_RDWR);
+		if (fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				devname, strerror(errno));
-		goto error;
+			goto error;
+		}
 	}
 
 	/* try mapping somewhere close to the end of hugepages */
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 458e6d0..828acc5 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -135,6 +135,8 @@  struct rte_pci_bus {
 
 /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
 #define RTE_PCI_DRV_NEED_MAPPING 0x0001
+/** Device needs PCI BAR mapping with enabled write combining (wc) */
+#define RTE_PCI_DRV_WC_ACTIVATE 0x0002
 /** Device driver supports link state interrupt */
 #define RTE_PCI_DRV_INTR_LSC	0x0008
 /** Device driver supports device removal interrupt */