[PATCH 0/4] support for write combining

[PATCH 0/4] support for write combining

[Rafal Kozik]

Support for write combining.

Rafal Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
 drivers/bus/pci/pci_common.c    | 13 ++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
 5 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/4] igb_uio: add wc option

[Rafal Kozik]

Write combining (wc) increase NIC performance by making better
utilization of PCI bus, but cannot be use by all PMD.

Parameter wc_activate add possibility to enable it for
those PMD that could support it.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
---
 kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index 4cae4dd..42e3b3f 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
 	int refcnt;
 };
 
+static int wc_activate;
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 /* sriov sysfs */
@@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	len = pci_resource_len(dev, pci_bar);
 	if (addr == 0 || len == 0)
 		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
+	if (wc_activate == 0) {
+		internal_addr = ioremap(addr, len);
+		if (internal_addr == NULL)
+			return -1;
+	} else {
+		internal_addr = NULL;
+	}
 	info->mem[n].name = name;
 	info->mem[n].addr = addr;
 	info->mem[n].internal_addr = internal_addr;
@@ -638,6 +643,12 @@ MODULE_PARM_DESC(intr_mode,
 "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
 "\n");
 
+module_param(wc_activate, int, 0);
+MODULE_PARM_DESC(wc_activate,
+"Activate support for write combining (WC) (default=0)\n"
+"    0 - disable\n"
+"    other - enable\n");
+
 MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
-- 
2.7.4

[PATCH 2/4] bus/pci: reference driver structure

[Rafal Kozik]

Reference driver structure before calling rte_pci_map_device.
It allow to use driver flags for adjusting configuration.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
---
 drivers/bus/pci/pci_common.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 2a00f36..15e9a47 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -159,17 +159,20 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
 		dev->id.device_id, dr->driver.name);
 
+	/* reference driver structure */
+	dev->driver = dr;
+	dev->device.driver = &dr->driver;
+
 	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
 		/* map resources for devices that use igb_uio */
 		ret = rte_pci_map_device(dev);
-		if (ret != 0)
+		if (ret != 0) {
+			dev->driver = NULL;
+			dev->device.driver = NULL;
 			return ret;
+		}
 	}
 
-	/* reference driver structure */
-	dev->driver = dr;
-	dev->device.driver = &dr->driver;
-
 	/* call the driver probe() function */
 	ret = dr->probe(dr, dev);
 	if (ret) {
-- 
2.7.4

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

[Rafal Kozik]

Write combining (wc) increase NIC performenca by making better
utilization of PCI bus, but cannot be used by all PMD.

It will be enable only if RTE_PCI_DRV_WC_ACTIVATE will be set in
drivers flags. For proper work also igb 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>
---
 drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index d423e4b..a1570c9 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -287,17 +287,14 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	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 357afb9..b7bcce3 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -132,6 +132,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 */
-- 
2.7.4

[PATCH 4/4] net/ena: enable WC

[Rafal Kozik]

Write combining (wc) increase NIC performenca by making better
utilization of PCI bus. ENA support this feature.

To enable it load igb driver with wc_activate set to 1.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
---
 drivers/net/ena/ena_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 34b2a8d..415d89d 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1889,7 +1889,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_ena_pmd = {
 	.id_table = pci_id_ena_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
+		     RTE_PCI_DRV_WC_ACTIVATE,
 	.probe = eth_ena_pci_probe,
 	.remove = eth_ena_pci_remove,
 };
-- 
2.7.4

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Wed, Apr 11, 2018 at 04:07:13PM +0200, Rafal Kozik wrote:
> Support for write combining.
> 
> Rafal Kozik (4):
>   igb_uio: add wc option
>   bus/pci: reference driver structure
>   eal: enable WC during resources mapping
>   net/ena: enable WC
> 
>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>  drivers/bus/pci/pci_common.c    | 13 ++++++++-----
>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>  drivers/net/ena/ena_ethdev.c    |  3 ++-
>  kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
>  5 files changed, 54 insertions(+), 20 deletions(-)
> 
Couple of thoughts on this set.

You add an option to the kernel module to allow wc to be supported on a
device, but when we go to do PCI mapping, we either map the regular
resource file or the _wc one. Therefore:

1. Why not always have igb_uio support write-combining since it can be
controlled thereafter via userspace mapping one file or another?

2. Why not always map both resource and resource_wc files at the PCI level,
and make them available via different pointers to the driver? Then the
driver can choose at the per-access level which it wants to use. For
example, for init of a device, a driver may do all register access via
uncachable memory, and only use the write-combined support for
performance-critical parts. [I have a draft patch lying around here
somewhere that does something similar to that.]

One last question - if using vfio-pci kernel module, do the resource_wc
files present the bars as write-combined memory type, or are they
uncachable?

Regards,
/Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

thank you for your advices.

> 1. Why not always have igb_uio support write-combining since it can be
> controlled thereafter via userspace mapping one file or another?

I added parameter to the igb_uio because currently it perform ioremap
and fails if it return NULL.
But performing ioremap makes it impossible to use WC, so I remove it.
ENA driver work well after this change, but I cannot test it on all
drivers and all platforms.
It seems to me that making it configurable prevents form spoiling
other drivers that could use internal_addr returned by ioremap.

> 2. Why not always map both resource and resource_wc files at the PCI level,
> and make them available via different pointers to the driver? Then the
> driver can choose at the per-access level which it wants to use. For
> example, for init of a device, a driver may do all register access via
> uncachable memory, and only use the write-combined support for
> performance-critical parts. [I have a draft patch lying around here
> somewhere that does something similar to that.]

I tried to implement this idea but without good results. I get mapping
with or without WC depending on mapping order.
As I was trying to find solution I come across with this paper:
https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
In section 5.3 and 5.4 it is discussing access to PCI resources.
According to it:

A request to uncached access can fail if there is already
an existing write-combine mapping for that region. A
request for write-combine access can succeed with un-
cached mapping instead, in the case of already existing
uncached mapping for this region.

We cannot use WC all the time, because it not guaranteed writing order.
On this basis I suppose that better option is to map each resource
only once depending on parameter provided by PMD.

> One last question - if using vfio-pci kernel module, do the resource_wc
> files present the bars as write-combined memory type, or are they
> uncachable?

I tried to use VFIO to map WC memory, but without success.

Best regards,
Rafal Kozik

2018-04-11 16:42 GMT+02:00 Bruce Richardson <bruce.richardson@intel.com>:
> On Wed, Apr 11, 2018 at 04:07:13PM +0200, Rafal Kozik wrote:
>> Support for write combining.
>>
>> Rafal Kozik (4):
>>   igb_uio: add wc option
>>   bus/pci: reference driver structure
>>   eal: enable WC during resources mapping
>>   net/ena: enable WC
>>
>>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>>  drivers/bus/pci/pci_common.c    | 13 ++++++++-----
>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>  drivers/net/ena/ena_ethdev.c    |  3 ++-
>>  kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
>>  5 files changed, 54 insertions(+), 20 deletions(-)
>>
> Couple of thoughts on this set.
>
> You add an option to the kernel module to allow wc to be supported on a
> device, but when we go to do PCI mapping, we either map the regular
> resource file or the _wc one. Therefore:
>
> 1. Why not always have igb_uio support write-combining since it can be
> controlled thereafter via userspace mapping one file or another?
>
> 2. Why not always map both resource and resource_wc files at the PCI level,
> and make them available via different pointers to the driver? Then the
> driver can choose at the per-access level which it wants to use. For
> example, for init of a device, a driver may do all register access via
> uncachable memory, and only use the write-combined support for
> performance-critical parts. [I have a draft patch lying around here
> somewhere that does something similar to that.]
>
> One last question - if using vfio-pci kernel module, do the resource_wc
> files present the bars as write-combined memory type, or are they
> uncachable?
>
> Regards,
> /Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

As from my last post passed ten days I would kindly ask if there are
any more comments?

Best regards,
Rafal Kozik

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Fri, Apr 27, 2018 at 10:27:02AM +0200, Rafał Kozik wrote:
> Hello Bruce,
> 
> As from my last post passed ten days I would kindly ask if there are
> any more comments?
> 
> Best regards,
> Rafal Kozik

Hi Rafal,

sorry for not responding before. The set makes a lot more sense to me now.
However, just one more question? If igb_uio is passed the flag to make
mappings wc, what happens if you have a mix of drivers that support or
don't support wc at the driver, not the hardware level. For example,
support there is one ena adapter and one i40e adapter. The hardware on both
should be mappable as WC, but one driver can use that, the other can't. How
is it handled?

/Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

It should work because decision about kind of mapping is made in patch
3 based on PMD request.

ENA use only one BAR in wc mode and two other without caching,
therefore not making remap in igb_uio rather not spoil anything.

I added patch 1 because this variable is provided also outside the
DPDK to Linux generic functions. I cannot test it with all drivers,
therefore I make it configurable.

But general combination of wc and not wc PMD should work, because of patch 3.
Also if WC is enabled by PMD (like in patch 4) not all BARs will by
mapped, but only those which has prefetchable flag enabled.

Best regards,
Rafal Kozik

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Mon, Apr 30, 2018 at 10:07:07AM +0200, Rafał Kozik wrote:
> Hello Bruce,
> 
> It should work because decision about kind of mapping is made in patch
> 3 based on PMD request.
> 
> ENA use only one BAR in wc mode and two other without caching,
> therefore not making remap in igb_uio rather not spoil anything.
> 
> I added patch 1 because this variable is provided also outside the
> DPDK to Linux generic functions. I cannot test it with all drivers,
> therefore I make it configurable.
> 
> But general combination of wc and not wc PMD should work, because of patch 3.
> Also if WC is enabled by PMD (like in patch 4) not all BARs will by
> mapped, but only those which has prefetchable flag enabled.
> 
Sounds good, so in that case I've no objection to the patch.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH 4/4] net/ena: enable WC

[Thomas Monjalon]

11/04/2018 16:07, Rafal Kozik:
> Write combining (wc) increase NIC performenca by making better
> utilization of PCI bus. ENA support this feature.
> 
> To enable it load igb driver with wc_activate set to 1.

typo: igb -> igb_uio

> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Please rebase this patch, thanks.

Re: [PATCH 1/4] igb_uio: add wc option

[Ferruh Yigit]

On 4/11/2018 3:07 PM, Rafal Kozik wrote:
> Write combining (wc) increase NIC performance by making better
> utilization of PCI bus, but cannot be use by all PMD.
> 
> Parameter wc_activate add possibility to enable it for
> those PMD that could support it.
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>

Hi Rafal,

First, sorry for delayed comment.

> ---
>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index 4cae4dd..42e3b3f 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>  	int refcnt;
>  };
>  
> +static int wc_activate;
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>  /* sriov sysfs */
> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>  	len = pci_resource_len(dev, pci_bar);
>  	if (addr == 0 || len == 0)
>  		return -1;
> -	internal_addr = ioremap(addr, len);
> -	if (internal_addr == NULL)
> -		return -1;
> +	if (wc_activate == 0) {
> +		internal_addr = ioremap(addr, len);
> +		if (internal_addr == NULL)
> +			return -1;
> +	} else {
> +		internal_addr = NULL;
> +	}

Why we need this update at all?
What is the relation between internal_address and write combined?

As far as I can see we are not using internal_addr at all, what do you observe
in write combined usage without this modification?

>  	info->mem[n].name = name;
>  	info->mem[n].addr = addr;
>  	info->mem[n].internal_addr = internal_addr;
> @@ -638,6 +643,12 @@ MODULE_PARM_DESC(intr_mode,
>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>  "\n");
>  
> +module_param(wc_activate, int, 0);
> +MODULE_PARM_DESC(wc_activate,
> +"Activate support for write combining (WC) (default=0)\n"
> +"    0 - disable\n"
> +"    other - enable\n");
> +
>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Intel Corporation");
> 

Re: [PATCH 2/4] bus/pci: reference driver structure

[Ferruh Yigit]

On 4/11/2018 3:07 PM, Rafal Kozik wrote:
> Reference driver structure before calling rte_pci_map_device.
> It allow to use driver flags for adjusting configuration.
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> ---
>  drivers/bus/pci/pci_common.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 2a00f36..15e9a47 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -159,17 +159,20 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>  	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
>  		dev->id.device_id, dr->driver.name);
>  
> +	/* reference driver structure */
> +	dev->driver = dr;
> +	dev->device.driver = &dr->driver;

It can be good to comment that this needs to be before rte_pci_map_device()
incase someone wants to refactor code later.

> +
>  	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
>  		/* map resources for devices that use igb_uio */
>  		ret = rte_pci_map_device(dev);
> -		if (ret != 0)
> +		if (ret != 0) {
> +			dev->driver = NULL;
> +			dev->device.driver = NULL;
>  			return ret;
> +		}
>  	}
>  
> -	/* reference driver structure */
> -	dev->driver = dr;
> -	dev->device.driver = &dr->driver;
> -
>  	/* call the driver probe() function */
>  	ret = dr->probe(dr, dev);
>  	if (ret) {
> 

Re: [PATCH 3/4] eal: enable WC during resources mapping

[Ferruh Yigit]

On 4/11/2018 3:07 PM, Rafal Kozik wrote:
> Write combining (wc) increase NIC performenca by making better
> utilization of PCI bus, but cannot be used by all PMD.
> 
> It will be enable only if RTE_PCI_DRV_WC_ACTIVATE will be set in
> drivers flags. For proper work also igb 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>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index d423e4b..a1570c9 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -287,17 +287,14 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	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) {

"fd" may be used uninitialized here, although its value doesn't affect the
branch taken it can be good to initialize it for any possible static analyzer
warning.

> +		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 357afb9..b7bcce3 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -132,6 +132,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 */
> 

Re: [PATCH 4/4] net/ena: enable WC

[Rafał Kozik]

Hello Thomas,

I will fix type, rebase and provide new patch set.

Best regards,
Rafal Kozik

2018-06-27 18:11 GMT+02:00 Thomas Monjalon <thomas@monjalon.net>:
> 11/04/2018 16:07, Rafal Kozik:
>> Write combining (wc) increase NIC performenca by making better
>> utilization of PCI bus. ENA support this feature.
>>
>> To enable it load igb driver with wc_activate set to 1.
>
> typo: igb -> igb_uio
>
>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> ---
>>  drivers/net/ena/ena_ethdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Please rebase this patch, thanks.
>
>

Re: [PATCH 2/4] bus/pci: reference driver structure

[Rafał Kozik]

2018-06-27 18:36 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> On 4/11/2018 3:07 PM, Rafal Kozik wrote:
>> Reference driver structure before calling rte_pci_map_device.
>> It allow to use driver flags for adjusting configuration.
>>
>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> ---
>>  drivers/bus/pci/pci_common.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
>> index 2a00f36..15e9a47 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -159,17 +159,20 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
>>       RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
>>               dev->id.device_id, dr->driver.name);
>>
>> +     /* reference driver structure */
>> +     dev->driver = dr;
>> +     dev->device.driver = &dr->driver;
>
> It can be good to comment that this needs to be before rte_pci_map_device()
> incase someone wants to refactor code later.
>

I will fix it in new, rebased patch set.

>> +
>>       if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
>>               /* map resources for devices that use igb_uio */
>>               ret = rte_pci_map_device(dev);
>> -             if (ret != 0)
>> +             if (ret != 0) {
>> +                     dev->driver = NULL;
>> +                     dev->device.driver = NULL;
>>                       return ret;
>> +             }
>>       }
>>
>> -     /* reference driver structure */
>> -     dev->driver = dr;
>> -     dev->device.driver = &dr->driver;
>> -
>>       /* call the driver probe() function */
>>       ret = dr->probe(dr, dev);
>>       if (ret) {
>>
>

Re: [PATCH 3/4] eal: enable WC during resources mapping

[Rafał Kozik]

2018-06-27 18:41 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> On 4/11/2018 3:07 PM, Rafal Kozik wrote:
>> Write combining (wc) increase NIC performenca by making better
>> utilization of PCI bus, but cannot be used by all PMD.
>>
>> It will be enable only if RTE_PCI_DRV_WC_ACTIVATE will be set in
>> drivers flags. For proper work also igb 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>
>> ---
>>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>  2 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
>> index d423e4b..a1570c9 100644
>> --- a/drivers/bus/pci/linux/pci_uio.c
>> +++ b/drivers/bus/pci/linux/pci_uio.c
>> @@ -287,17 +287,14 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>>       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) {
>
> "fd" may be used uninitialized here, although its value doesn't affect the
> branch taken it can be good to initialize it for any possible static analyzer
> warning.
>

I will fix it in new, rebased patch set.

>> +             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 357afb9..b7bcce3 100644
>> --- a/drivers/bus/pci/rte_bus_pci.h
>> +++ b/drivers/bus/pci/rte_bus_pci.h
>> @@ -132,6 +132,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 */
>>
>

Re: [PATCH 1/4] igb_uio: add wc option

[Rafał Kozik]

2018-06-27 18:34 GMT+02:00 Ferruh Yigit <ferruh.yigit@intel.com>:
> On 4/11/2018 3:07 PM, Rafal Kozik wrote:
>> Write combining (wc) increase NIC performance by making better
>> utilization of PCI bus, but cannot be use by all PMD.
>>
>> Parameter wc_activate add possibility to enable it for
>> those PMD that could support it.
>>
>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>
> Hi Rafal,
>
> First, sorry for delayed comment.
>
>> ---
>>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> index 4cae4dd..42e3b3f 100644
>> --- a/kernel/linux/igb_uio/igb_uio.c
>> +++ b/kernel/linux/igb_uio/igb_uio.c
>> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>>       int refcnt;
>>  };
>>
>> +static int wc_activate;
>>  static char *intr_mode;
>>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>>  /* sriov sysfs */
>> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>>       len = pci_resource_len(dev, pci_bar);
>>       if (addr == 0 || len == 0)
>>               return -1;
>> -     internal_addr = ioremap(addr, len);
>> -     if (internal_addr == NULL)
>> -             return -1;
>> +     if (wc_activate == 0) {
>> +             internal_addr = ioremap(addr, len);
>> +             if (internal_addr == NULL)
>> +                     return -1;
>> +     } else {
>> +             internal_addr = NULL;
>> +     }
>
> Why we need this update at all?
> What is the relation between internal_address and write combined?
>
> As far as I can see we are not using internal_addr at all, what do you observe
> in write combined usage without this modification?
>

To get internal_addr memory need to be mapped. But as memory could not be
mapped twice: with and without WC it should be skipped for WC. [1]
WC does not work without this modification.

[1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
        section 5.3 and 5.4

>>       info->mem[n].name = name;
>>       info->mem[n].addr = addr;
>>       info->mem[n].internal_addr = internal_addr;
>> @@ -638,6 +643,12 @@ MODULE_PARM_DESC(intr_mode,
>>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>>  "\n");
>>
>> +module_param(wc_activate, int, 0);
>> +MODULE_PARM_DESC(wc_activate,
>> +"Activate support for write combining (WC) (default=0)\n"
>> +"    0 - disable\n"
>> +"    other - enable\n");
>> +
>>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>>  MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Intel Corporation");
>>
>

[PATCH v2 0/4] support for write combining

[Rafal Kozik]

Support for write combining.

---
v2:
 * Rebased on top of master.
 * Fix typos.
 * Make commit messages more verbose.
 * Add comments.
 * Initialize fd.

Rafal Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++------------
 drivers/bus/pci/pci_common.c    | 17 ++++++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
 5 files changed, 59 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] igb_uio: add wc option

[Rafal Kozik]

Write combining (WC) increases NIC performance by making better
utilization of PCI bus, but cannot be use by all PMDs.

To get internal_addr memory need to be mapped. But as memory could not be
mapped twice: with and without WC, it should be skipped for WC. [1]

To do not spoil other drivers that potentially could use internal_addr,
parameter wc_activate adds possibility to skip it for those PMDs,
that do not use it.

[1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
	section 5.3 and 5.4

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index b3233f1..3382fb1 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
 	int refcnt;
 };
 
+static int wc_activate;
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 /* sriov sysfs */
@@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	len = pci_resource_len(dev, pci_bar);
 	if (addr == 0 || len == 0)
 		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
+	if (wc_activate == 0) {
+		internal_addr = ioremap(addr, len);
+		if (internal_addr == NULL)
+			return -1;
+	} else {
+		internal_addr = NULL;
+	}
 	info->mem[n].name = name;
 	info->mem[n].addr = addr;
 	info->mem[n].internal_addr = internal_addr;
@@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
 "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
 "\n");
 
+module_param(wc_activate, int, 0);
+MODULE_PARM_DESC(wc_activate,
+"Activate support for write combining (WC) (default=0)\n"
+"    0 - disable\n"
+"    other - enable\n");
+
 MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
-- 
2.7.4

[PATCH v2 2/4] bus/pci: reference driver structure

[Rafal Kozik]

Add pointer to driver structure before calling rte_pci_map_device.
It allows to use driver flags for adjusting configuration.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

---
 drivers/bus/pci/pci_common.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index d8151b0..8f5d77f 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -158,17 +158,24 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
 		dev->id.device_id, dr->driver.name);
 
+	/*
+	 * reference driver structure
+	 * This need to be before rte_pci_map_device(), as it enable to use
+	 * driver flags for adjusting configuration.
+	 */
+	dev->driver = dr;
+	dev->device.driver = &dr->driver;
+
 	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
 		/* map resources for devices that use igb_uio */
 		ret = rte_pci_map_device(dev);
-		if (ret != 0)
+		if (ret != 0) {
+			dev->driver = NULL;
+			dev->device.driver = NULL;
 			return ret;
+		}
 	}
 
-	/* reference driver structure */
-	dev->driver = dr;
-	dev->device.driver = &dr->driver;
-
 	/* call the driver probe() function */
 	ret = dr->probe(dr, dev);
 	if (ret) {
-- 
2.7.4

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

[Rafal Kozik]

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);
+	}
+
+	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 */
-- 
2.7.4

[PATCH v2 4/4] net/ena: enable WC

[Rafal Kozik]

Write combining (WC) increases NIC performance by making better
utilization of PCI bus. ENA PMD may make usage of this feature.

To enable it load igb_uio driver with wc_activate set to 1.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ena/ena_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 9ae73e3..1870edf 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2210,7 +2210,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_ena_pmd = {
 	.id_table = pci_id_ena_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
+		     RTE_PCI_DRV_WC_ACTIVATE,
 	.probe = eth_ena_pci_probe,
 	.remove = eth_ena_pci_remove,
 };
-- 
2.7.4

Re: [PATCH v2 1/4] igb_uio: add wc option

[Ferruh Yigit]

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 use by all PMDs.
> 
> To get internal_addr memory need to be mapped. But as memory could not be
> mapped twice: with and without WC, it should be skipped for WC. [1]
> 
> To do not spoil other drivers that potentially could use internal_addr,
> parameter wc_activate adds possibility to skip it for those PMDs,
> that do not use it.
> 
> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
> 	section 5.3 and 5.4

Hi Rafal,

Thank you for more information but I have a few more question:

- What do you mean "But as memory could not be mapped twice: with and without WC"?

ioremap() maps the physical address for kernel usage, and via uio we are mapping
it to userspace, do you mean these two?

- "internal_addr" is should be for kernel sage not for DPDK drivers which are in
the userspace, why it is a concern for us?

- What happens if you don't update this code at all? Won't you able to map
device address into userspace?
I tested adding RTE_PCI_DRV_WC_ACTIVATE to i40e, on top of your patch, and able
to map without igb_uio update.
I am not able to understand need of the modification.

> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..3382fb1 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>  	int refcnt;
>  };
>  
> +static int wc_activate;
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>  /* sriov sysfs */
> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>  	len = pci_resource_len(dev, pci_bar);
>  	if (addr == 0 || len == 0)
>  		return -1;
> -	internal_addr = ioremap(addr, len);
> -	if (internal_addr == NULL)
> -		return -1;
> +	if (wc_activate == 0) {
> +		internal_addr = ioremap(addr, len);
> +		if (internal_addr == NULL)
> +			return -1;
> +	} else {
> +		internal_addr = NULL;
> +	}
>  	info->mem[n].name = name;
>  	info->mem[n].addr = addr;
>  	info->mem[n].internal_addr = internal_addr;
> @@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>  "\n");
>  
> +module_param(wc_activate, int, 0);
> +MODULE_PARM_DESC(wc_activate,
> +"Activate support for write combining (WC) (default=0)\n"
> +"    0 - disable\n"
> +"    other - enable\n");
> +
>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Intel Corporation");
> 

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

[Ferruh Yigit]

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

Re: [PATCH v2 1/4] igb_uio: add wc option

[Rafał Kozik]

2018-06-28 16:32 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 use by all PMDs.
>>
>> To get internal_addr memory need to be mapped. But as memory could not be
>> mapped twice: with and without WC, it should be skipped for WC. [1]
>>
>> To do not spoil other drivers that potentially could use internal_addr,
>> parameter wc_activate adds possibility to skip it for those PMDs,
>> that do not use it.
>>
>> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
>>       section 5.3 and 5.4
>
> Hi Rafal,
>
> Thank you for more information but I have a few more question:
>
> - What do you mean "But as memory could not be mapped twice: with and without WC"?
>
> ioremap() maps the physical address for kernel usage, and via uio we are mapping
> it to userspace, do you mean these two?
>
> - "internal_addr" is should be for kernel sage not for DPDK drivers which are in
> the userspace, why it is a concern for us?
>
> - What happens if you don't update this code at all? Won't you able to map
> device address into userspace?
> I tested adding RTE_PCI_DRV_WC_ACTIVATE to i40e, on top of your patch, and able
> to map without igb_uio update.
> I am not able to understand need of the modification.
>

Hello Ferruh,

I was not precisely. Memory could be mapped multiple time,
but cannot be mapped with and without WC support simultaneously.
When not setting wc_activate memory mapping work, but silently
fall-back to non prefetchable mode.

I perform measurements of writing speed.
When parameter wc_activate was set I get 4.81 GB/s.
Without this parameter result was 0.07 GB/s.
Code used for testing is located here:
gist.github.com/semihalf-kozik-rafal/327208cd52a2fac2d12250028becf9b3

Best regards,
Rafal

>>
>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>> index b3233f1..3382fb1 100644
>> --- a/kernel/linux/igb_uio/igb_uio.c
>> +++ b/kernel/linux/igb_uio/igb_uio.c
>> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>>       int refcnt;
>>  };
>>
>> +static int wc_activate;
>>  static char *intr_mode;
>>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>>  /* sriov sysfs */
>> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>>       len = pci_resource_len(dev, pci_bar);
>>       if (addr == 0 || len == 0)
>>               return -1;
>> -     internal_addr = ioremap(addr, len);
>> -     if (internal_addr == NULL)
>> -             return -1;
>> +     if (wc_activate == 0) {
>> +             internal_addr = ioremap(addr, len);
>> +             if (internal_addr == NULL)
>> +                     return -1;
>> +     } else {
>> +             internal_addr = NULL;
>> +     }
>>       info->mem[n].name = name;
>>       info->mem[n].addr = addr;
>>       info->mem[n].internal_addr = internal_addr;
>> @@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
>>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>>  "\n");
>>
>> +module_param(wc_activate, int, 0);
>> +MODULE_PARM_DESC(wc_activate,
>> +"Activate support for write combining (WC) (default=0)\n"
>> +"    0 - disable\n"
>> +"    other - enable\n");
>> +
>>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>>  MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Intel Corporation");
>>
>

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

[Rafał Kozik]

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

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

[Ferruh Yigit]

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

Re: [PATCH v2 1/4] igb_uio: add wc option

[Ferruh Yigit]

On 6/29/2018 9:35 AM, Rafał Kozik wrote:
> 2018-06-28 16:32 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 use by all PMDs.
>>>
>>> To get internal_addr memory need to be mapped. But as memory could not be
>>> mapped twice: with and without WC, it should be skipped for WC. [1]
>>>
>>> To do not spoil other drivers that potentially could use internal_addr,
>>> parameter wc_activate adds possibility to skip it for those PMDs,
>>> that do not use it.
>>>
>>> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
>>>       section 5.3 and 5.4
>>
>> Hi Rafal,
>>
>> Thank you for more information but I have a few more question:
>>
>> - What do you mean "But as memory could not be mapped twice: with and without WC"?
>>
>> ioremap() maps the physical address for kernel usage, and via uio we are mapping
>> it to userspace, do you mean these two?
>>
>> - "internal_addr" is should be for kernel sage not for DPDK drivers which are in
>> the userspace, why it is a concern for us?
>>
>> - What happens if you don't update this code at all? Won't you able to map
>> device address into userspace?
>> I tested adding RTE_PCI_DRV_WC_ACTIVATE to i40e, on top of your patch, and able
>> to map without igb_uio update.
>> I am not able to understand need of the modification.
>>
> 
> Hello Ferruh,
> 
> I was not precisely. Memory could be mapped multiple time,
> but cannot be mapped with and without WC support simultaneously.
> When not setting wc_activate memory mapping work, but silently
> fall-back to non prefetchable mode.

How can I confirm this silently fall-back behavior, is there any log can I turn
on in kernel or anything from proc/sysfs?

> 
> I perform measurements of writing speed.
> When parameter wc_activate was set I get 4.81 GB/s.
> Without this parameter result was 0.07 GB/s.
> Code used for testing is located here:
> gist.github.com/semihalf-kozik-rafal/327208cd52a2fac2d12250028becf9b3
> 
> Best regards,
> Rafal
> 
>>>
>>> Signed-off-by: Rafal Kozik <rk@semihalf.com>
>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
>>> index b3233f1..3382fb1 100644
>>> --- a/kernel/linux/igb_uio/igb_uio.c
>>> +++ b/kernel/linux/igb_uio/igb_uio.c
>>> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>>>       int refcnt;
>>>  };
>>>
>>> +static int wc_activate;
>>>  static char *intr_mode;
>>>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>>>  /* sriov sysfs */
>>> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>>>       len = pci_resource_len(dev, pci_bar);
>>>       if (addr == 0 || len == 0)
>>>               return -1;
>>> -     internal_addr = ioremap(addr, len);
>>> -     if (internal_addr == NULL)
>>> -             return -1;
>>> +     if (wc_activate == 0) {
>>> +             internal_addr = ioremap(addr, len);
>>> +             if (internal_addr == NULL)
>>> +                     return -1;
>>> +     } else {
>>> +             internal_addr = NULL;
>>> +     }
>>>       info->mem[n].name = name;
>>>       info->mem[n].addr = addr;
>>>       info->mem[n].internal_addr = internal_addr;
>>> @@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
>>>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>>>  "\n");
>>>
>>> +module_param(wc_activate, int, 0);
>>> +MODULE_PARM_DESC(wc_activate,
>>> +"Activate support for write combining (WC) (default=0)\n"
>>> +"    0 - disable\n"
>>> +"    other - enable\n");
>>> +
>>>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_AUTHOR("Intel Corporation");
>>>
>>

[PATCH v3 0/4] support for write combining

[Rafal Kozik]

Support for write combining.

---
v2:
 * Rebased on top of master.
 * Fix typos.
 * Make commit messages more verbose.
 * Add comments.
 * Initialize fd.

---
v3:
 * Log if BAR was mapped with or without support for WC.

Rafal Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 44 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/pci_common.c    | 17 +++++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 17 +++++++++++++---
 5 files changed, 62 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] igb_uio: add wc option

[Rafal Kozik]

Write combining (WC) increases NIC performance by making better
utilization of PCI bus, but cannot be use by all PMDs.

To get internal_addr memory need to be mapped. But as memory could not be
mapped twice: with and without WC, it should be skipped for WC. [1]

To do not spoil other drivers that potentially could use internal_addr,
parameter wc_activate adds possibility to skip it for those PMDs,
that do not use it.

[1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
	section 5.3 and 5.4

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index b3233f1..3382fb1 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
 	int refcnt;
 };
 
+static int wc_activate;
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 /* sriov sysfs */
@@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	len = pci_resource_len(dev, pci_bar);
 	if (addr == 0 || len == 0)
 		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
+	if (wc_activate == 0) {
+		internal_addr = ioremap(addr, len);
+		if (internal_addr == NULL)
+			return -1;
+	} else {
+		internal_addr = NULL;
+	}
 	info->mem[n].name = name;
 	info->mem[n].addr = addr;
 	info->mem[n].internal_addr = internal_addr;
@@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
 "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
 "\n");
 
+module_param(wc_activate, int, 0);
+MODULE_PARM_DESC(wc_activate,
+"Activate support for write combining (WC) (default=0)\n"
+"    0 - disable\n"
+"    other - enable\n");
+
 MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
-- 
2.7.4

[PATCH v3 2/4] bus/pci: reference driver structure

[Rafal Kozik]

Add pointer to driver structure before calling rte_pci_map_device.
It allows to use driver flags for adjusting configuration.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/pci/pci_common.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index d8151b0..8f5d77f 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -158,17 +158,24 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
 		dev->id.device_id, dr->driver.name);
 
+	/*
+	 * reference driver structure
+	 * This need to be before rte_pci_map_device(), as it enable to use
+	 * driver flags for adjusting configuration.
+	 */
+	dev->driver = dr;
+	dev->device.driver = &dr->driver;
+
 	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
 		/* map resources for devices that use igb_uio */
 		ret = rte_pci_map_device(dev);
-		if (ret != 0)
+		if (ret != 0) {
+			dev->driver = NULL;
+			dev->device.driver = NULL;
 			return ret;
+		}
 	}
 
-	/* reference driver structure */
-	dev->driver = dr;
-	dev->device.driver = &dr->driver;
-
 	/* call the driver probe() function */
 	ret = dr->probe(dr, dev);
 	if (ret) {
-- 
2.7.4

[PATCH v3 3/4] eal: enable WC during resources mapping

[Rafal Kozik]

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 | 44 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index d423e4b..fb02f0a 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,34 @@ 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 (fd >= 0)
+			RTE_LOG(INFO, EAL, "%s mapped\n", devname);
+	}
+
+	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;
+		}
+		RTE_LOG(INFO, EAL, "%s mapped\n", devname);
 	}
 
 	/* 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 */
-- 
2.7.4

[PATCH v3 4/4] net/ena: enable WC

[Rafal Kozik]

Write combining (WC) increases NIC performance by making better
utilization of PCI bus. ENA PMD may make usage of this feature.

To enable it load igb_uio driver with wc_activate set to 1.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ena/ena_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 9ae73e3..1870edf 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2210,7 +2210,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_ena_pmd = {
 	.id_table = pci_id_ena_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
+		     RTE_PCI_DRV_WC_ACTIVATE,
 	.probe = eth_ena_pci_probe,
 	.remove = eth_ena_pci_remove,
 };
-- 
2.7.4

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

[Rafał Kozik]

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

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

[Ferruh Yigit]

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

Re: [PATCH v3 1/4] igb_uio: add wc option

[Ferruh Yigit]

On 6/29/2018 11:24 AM, Rafal Kozik wrote:
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus, but cannot be use by all PMDs.
> 
> To get internal_addr memory need to be mapped. But as memory could not be
> mapped twice: with and without WC, it should be skipped for WC. [1]
> 
> To do not spoil other drivers that potentially could use internal_addr,
> parameter wc_activate adds possibility to skip it for those PMDs,
> that do not use it.
> 
> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
> 	section 5.3 and 5.4
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  kernel/linux/igb_uio/igb_uio.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..3382fb1 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>  	int refcnt;
>  };
>  
> +static int wc_activate;
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>  /* sriov sysfs */
> @@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>  	len = pci_resource_len(dev, pci_bar);
>  	if (addr == 0 || len == 0)
>  		return -1;
> -	internal_addr = ioremap(addr, len);
> -	if (internal_addr == NULL)
> -		return -1;
> +	if (wc_activate == 0) {
> +		internal_addr = ioremap(addr, len);
> +		if (internal_addr == NULL)
> +			return -1;
> +	} else {
> +		internal_addr = NULL;

Similar to previous comment, would you mind add log here when wc_activate set.
This helps user to verify/check what requested.

> +	}
>  	info->mem[n].name = name;
>  	info->mem[n].addr = addr;
>  	info->mem[n].internal_addr = internal_addr;
> @@ -650,6 +655,12 @@ MODULE_PARM_DESC(intr_mode,
>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>  "\n");
>  
> +module_param(wc_activate, int, 0);
> +MODULE_PARM_DESC(wc_activate,
> +"Activate support for write combining (WC) (default=0)\n"
> +"    0 - disable\n"
> +"    other - enable\n");
> +
>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Intel Corporation");
> 

[PATCH v4 0/4] Support for write combining.

[Rafal Kozik]

Support for write combining.

---
v2:
 * Rebased on top of master.
 * Fix typos.
 * Make commit messages more verbose.
 * Add comments.
 * Initialize fd.

---
v3:
 * Log if BAR was mapped with or without support for WC.

---
v4:
 * Before opening PCI resource, check if it supports WC.
 * Log only when WC mapping failed.
 * Log when wc_activate is set in igb_uio driver.

Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 47 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/pci_common.c    | 17 ++++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 18 +++++++++++++---
 5 files changed, 66 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] igb_uio: add wc option

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Write combining (WC) increases NIC performance by making better
utilization of PCI bus, but cannot be use by all PMD.

To get internal_addr memory need to be mapped. But as memory could not be
mapped twice: with and without WC it should be skipped for WC. [1]

To do not spoil other drivers that potentially could use internal_addr,
parameter wc_activate adds possibility to skip it for those PMDs,
that do not use it.

[1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
	section 5.3 and 5.4

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index b3233f1..e16e760 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
 	int refcnt;
 };
 
+static int wc_activate;
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 /* sriov sysfs */
@@ -375,9 +376,14 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	len = pci_resource_len(dev, pci_bar);
 	if (addr == 0 || len == 0)
 		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
+	if (wc_activate == 0) {
+		internal_addr = ioremap(addr, len);
+		if (internal_addr == NULL)
+			return -1;
+	} else {
+		internal_addr = NULL;
+		pr_info("wc_activate is set\n");
+	}
 	info->mem[n].name = name;
 	info->mem[n].addr = addr;
 	info->mem[n].internal_addr = internal_addr;
@@ -650,6 +656,12 @@ MODULE_PARM_DESC(intr_mode,
 "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
 "\n");
 
+module_param(wc_activate, int, 0);
+MODULE_PARM_DESC(wc_activate,
+"Activate support for write combining (WC) (default=0)\n"
+"    0 - disable\n"
+"    other - enable\n");
+
 MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
-- 
2.7.4

[PATCH v4 2/4] bus/pci: reference driver structure

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Add pointer to driver structure before calling rte_pci_map_device.
It allows to use driver flags for adjusting configuration.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/pci/pci_common.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index d8151b0..8f5d77f 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -158,17 +158,24 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
 		dev->id.device_id, dr->driver.name);
 
+	/*
+	 * reference driver structure
+	 * This need to be before rte_pci_map_device(), as it enable to use
+	 * driver flags for adjusting configuration.
+	 */
+	dev->driver = dr;
+	dev->device.driver = &dr->driver;
+
 	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
 		/* map resources for devices that use igb_uio */
 		ret = rte_pci_map_device(dev);
-		if (ret != 0)
+		if (ret != 0) {
+			dev->driver = NULL;
+			dev->device.driver = NULL;
 			return ret;
+		}
 	}
 
-	/* reference driver structure */
-	dev->driver = dr;
-	dev->device.driver = &dr->driver;
-
 	/* call the driver probe() function */
 	ret = dr->probe(dr, dev);
 	if (ret) {
-- 
2.7.4

[PATCH v4 3/4] eal: enable WC during resources mapping

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

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 check if it supports WC
and try to use it.
In case of failure, it will fall-back 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 | 47 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index d423e4b..a7c1442 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,37 @@ 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);
+
+		if (access(devname, R_OK|W_OK) != -1) {
+			fd = open(devname, O_RDWR);
+			if (fd < 0)
+				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
+					"Fall-back to non prefetchable mode.\n",
+					devname);
+		}
+	}
+
+	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 */
-- 
2.7.4

[PATCH v4 4/4] net/ena: enable WC

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Write combining (WC) increases NIC performance by making better
utilization of PCI bus. ENA PMD may make usage of this feature.

To enable it load igb_uio driver with wc_activate set to 1.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ena/ena_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 9ae73e3..1870edf 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2210,7 +2210,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_ena_pmd = {
 	.id_table = pci_id_ena_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
+		     RTE_PCI_DRV_WC_ACTIVATE,
 	.probe = eth_ena_pci_probe,
 	.remove = eth_ena_pci_remove,
 };
-- 
2.7.4

Re: [PATCH v4 1/4] igb_uio: add wc option

[Rafał Kozik]

> How can I confirm this silently fall-back behavior, is there any log can I turn
> on in kernel or anything from proc/sysfs?

I cannot find any.
I check it by measuring write speed.

2018-06-29 14:17 GMT+02:00 Rafal Kozik <rk@semihalf.com>:
> From: Kozik <rk@semihalf.com>
>
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus, but cannot be use by all PMD.
>
> To get internal_addr memory need to be mapped. But as memory could not be
> mapped twice: with and without WC it should be skipped for WC. [1]
>
> To do not spoil other drivers that potentially could use internal_addr,
> parameter wc_activate adds possibility to skip it for those PMDs,
> that do not use it.
>
> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
>         section 5.3 and 5.4
>
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  kernel/linux/igb_uio/igb_uio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..e16e760 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>         int refcnt;
>  };
>
> +static int wc_activate;
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>  /* sriov sysfs */
> @@ -375,9 +376,14 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>         len = pci_resource_len(dev, pci_bar);
>         if (addr == 0 || len == 0)
>                 return -1;
> -       internal_addr = ioremap(addr, len);
> -       if (internal_addr == NULL)
> -               return -1;
> +       if (wc_activate == 0) {
> +               internal_addr = ioremap(addr, len);
> +               if (internal_addr == NULL)
> +                       return -1;
> +       } else {
> +               internal_addr = NULL;
> +               pr_info("wc_activate is set\n");
> +       }
>         info->mem[n].name = name;
>         info->mem[n].addr = addr;
>         info->mem[n].internal_addr = internal_addr;
> @@ -650,6 +656,12 @@ MODULE_PARM_DESC(intr_mode,
>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>  "\n");
>
> +module_param(wc_activate, int, 0);
> +MODULE_PARM_DESC(wc_activate,
> +"Activate support for write combining (WC) (default=0)\n"
> +"    0 - disable\n"
> +"    other - enable\n");
> +
>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Intel Corporation");
> --
> 2.7.4
>

Re: [PATCH v4 1/4] igb_uio: add wc option

[Ferruh Yigit]

On 6/29/2018 1:17 PM, Rafal Kozik wrote:
> From: Kozik <rk@semihalf.com>
> 
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus, but cannot be use by all PMD.
> 
> To get internal_addr memory need to be mapped. But as memory could not be
> mapped twice: with and without WC it should be skipped for WC. [1]
> 
> To do not spoil other drivers that potentially could use internal_addr,
> parameter wc_activate adds possibility to skip it for those PMDs,
> that do not use it.
> 
> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
> 	section 5.3 and 5.4
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

(I am still not able to confirm this is needed, but since the behavior change is
controlled by module param and internal_addr is not used at all, no need to
block the patch)

Re: [PATCH v4 1/4] igb_uio: add wc option

[Ferruh Yigit]

On 6/29/2018 1:17 PM, Rafal Kozik wrote:
> From: Kozik <rk@semihalf.com>
> 
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus, but cannot be use by all PMD.
> 
> To get internal_addr memory need to be mapped. But as memory could not be
> mapped twice: with and without WC it should be skipped for WC. [1]
> 
> To do not spoil other drivers that potentially could use internal_addr,
> parameter wc_activate adds possibility to skip it for those PMDs,
> that do not use it.
> 
> [1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
> 	section 5.3 and 5.4
> 
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  kernel/linux/igb_uio/igb_uio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..e16e760 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
>  	int refcnt;
>  };
>  
> +static int wc_activate;
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
>  /* sriov sysfs */
> @@ -375,9 +376,14 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
>  	len = pci_resource_len(dev, pci_bar);
>  	if (addr == 0 || len == 0)
>  		return -1;
> -	internal_addr = ioremap(addr, len);
> -	if (internal_addr == NULL)
> -		return -1;
> +	if (wc_activate == 0) {
> +		internal_addr = ioremap(addr, len);
> +		if (internal_addr == NULL)
> +			return -1;
> +	} else {
> +		internal_addr = NULL;
> +		pr_info("wc_activate is set\n");

This location has been reached during device probe, so this log has been printed
for each BAR of each device, which is unnecessary.
My intention was print the log once when wc_activate is set, similar to
"intr_mode", may bad that I point to wrong place,
Would you mind making one more version to fix this, you can keep the ack.

Thanks,
ferruh


> +	}
>  	info->mem[n].name = name;
>  	info->mem[n].addr = addr;
>  	info->mem[n].internal_addr = internal_addr;
> @@ -650,6 +656,12 @@ MODULE_PARM_DESC(intr_mode,
>  "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
>  "\n");
>  
> +module_param(wc_activate, int, 0);
> +MODULE_PARM_DESC(wc_activate,
> +"Activate support for write combining (WC) (default=0)\n"
> +"    0 - disable\n"
> +"    other - enable\n");
> +
>  MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Intel Corporation");
> 

[PATCH v5 0/4] Support for write combining.

[Rafal Kozik]

Support for write combining.

---
v2:
 * Rebased on top of master.
 * Fix typos.
 * Make commit messages more verbose.
 * Add comments.
 * Initialize fd.

---
v3:
 * Log if BAR was mapped with or without support for WC.

---
v4:
 * Log only if WC mapping failed.
 * Log if wc_activate is set in igb_uio driver.

---
v5:
 * Log message in igb_uio will be printed only once.

Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 47 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/pci_common.c    | 17 ++++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 20 +++++++++++++++---
 5 files changed, 68 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v5 1/4] igb_uio: add wc option

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Write combining (WC) increases NIC performance by making better
utilization of PCI bus, but cannot be use by all PMD.

To get internal_addr memory need to be mapped. But as memory could not be
mapped twice: with and without WC it should be skipped for WC. [1]

To do not spoil other drivers that potentially could use internal_addr,
parameter wc_activate adds possibility to skip it for those PMDs,
that do not use it.

[1] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
	section 5.3 and 5.4

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 kernel/linux/igb_uio/igb_uio.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index b3233f1..3398eac 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -30,6 +30,7 @@ struct rte_uio_pci_dev {
 	int refcnt;
 };
 
+static int wc_activate;
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
 /* sriov sysfs */
@@ -375,9 +376,13 @@ igbuio_pci_setup_iomem(struct pci_dev *dev, struct uio_info *info,
 	len = pci_resource_len(dev, pci_bar);
 	if (addr == 0 || len == 0)
 		return -1;
-	internal_addr = ioremap(addr, len);
-	if (internal_addr == NULL)
-		return -1;
+	if (wc_activate == 0) {
+		internal_addr = ioremap(addr, len);
+		if (internal_addr == NULL)
+			return -1;
+	} else {
+		internal_addr = NULL;
+	}
 	info->mem[n].name = name;
 	info->mem[n].addr = addr;
 	info->mem[n].internal_addr = internal_addr;
@@ -626,6 +631,9 @@ igbuio_pci_init_module(void)
 		return -EINVAL;
 	}
 
+	if (wc_activate != 0)
+		pr_info("wc_activate is set\n");
+
 	ret = igbuio_config_intr_mode(intr_mode);
 	if (ret < 0)
 		return ret;
@@ -650,6 +658,12 @@ MODULE_PARM_DESC(intr_mode,
 "    " RTE_INTR_MODE_LEGACY_NAME "     Use Legacy interrupt\n"
 "\n");
 
+module_param(wc_activate, int, 0);
+MODULE_PARM_DESC(wc_activate,
+"Activate support for write combining (WC) (default=0)\n"
+"    0 - disable\n"
+"    other - enable\n");
+
 MODULE_DESCRIPTION("UIO driver for Intel IGB PCI cards");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Intel Corporation");
-- 
2.7.4

[PATCH v5 2/4] bus/pci: reference driver structure

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Add pointer to driver structure before calling rte_pci_map_device.
It allows to use driver flags for adjusting configuration.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/pci/pci_common.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index d8151b0..8f5d77f 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -158,17 +158,24 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	RTE_LOG(INFO, EAL, "  probe driver: %x:%x %s\n", dev->id.vendor_id,
 		dev->id.device_id, dr->driver.name);
 
+	/*
+	 * reference driver structure
+	 * This need to be before rte_pci_map_device(), as it enable to use
+	 * driver flags for adjusting configuration.
+	 */
+	dev->driver = dr;
+	dev->device.driver = &dr->driver;
+
 	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) {
 		/* map resources for devices that use igb_uio */
 		ret = rte_pci_map_device(dev);
-		if (ret != 0)
+		if (ret != 0) {
+			dev->driver = NULL;
+			dev->device.driver = NULL;
 			return ret;
+		}
 	}
 
-	/* reference driver structure */
-	dev->driver = dr;
-	dev->device.driver = &dr->driver;
-
 	/* call the driver probe() function */
 	ret = dr->probe(dr, dev);
 	if (ret) {
-- 
2.7.4

[PATCH v5 3/4] eal: enable WC during resources mapping

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

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 check if it support WC
and then try to us it.
In case of failure, 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 | 47 ++++++++++++++++++++++++++++++-----------
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index d423e4b..a7c1442 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,37 @@ 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);
+
+		if (access(devname, R_OK|W_OK) != -1) {
+			fd = open(devname, O_RDWR);
+			if (fd < 0)
+				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
+					"Fall-back to non prefetchable mode.\n",
+					devname);
+		}
+	}
+
+	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 */
-- 
2.7.4

[PATCH v5 4/4] net/ena: enable WC

[Rafal Kozik]

From: Kozik <rk@semihalf.com>

Write combining (WC) increases NIC performance by making better
utilization of PCI bus. ENA PMD may make usage of this feature.

To enable it load igb_uio driver with wc_activate set to 1.

Signed-off-by: Rafal Kozik <rk@semihalf.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ena/ena_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 9ae73e3..1870edf 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2210,7 +2210,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
 
 static struct rte_pci_driver rte_ena_pmd = {
 	.id_table = pci_id_ena_map,
-	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
+	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
+		     RTE_PCI_DRV_WC_ACTIVATE,
 	.probe = eth_ena_pci_probe,
 	.remove = eth_ena_pci_remove,
 };
-- 
2.7.4

Re: [PATCH v5 0/4] Support for write combining.

[Ferruh Yigit]

On 6/29/2018 2:54 PM, Rafal Kozik wrote:
> Support for write combining.
> 
> ---
> v2:
>  * Rebased on top of master.
>  * Fix typos.
>  * Make commit messages more verbose.
>  * Add comments.
>  * Initialize fd.
> 
> ---
> v3:
>  * Log if BAR was mapped with or without support for WC.
> 
> ---
> v4:
>  * Log only if WC mapping failed.
>  * Log if wc_activate is set in igb_uio driver.
> 
> ---
> v5:
>  * Log message in igb_uio will be printed only once.
> 
> Kozik (4):
>   igb_uio: add wc option
>   bus/pci: reference driver structure
>   eal: enable WC during resources mapping
>   net/ena: enable WC

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [PATCH v5 0/4] Support for write combining.

[Thomas Monjalon]

29/06/2018 16:26, Ferruh Yigit:
> On 6/29/2018 2:54 PM, Rafal Kozik wrote:
> > Support for write combining.
> > 
> > Kozik (4):
> >   igb_uio: add wc option
> >   bus/pci: reference driver structure
> >   eal: enable WC during resources mapping
> >   net/ena: enable WC
> 
> For series,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks

Re: [PATCH v2 4/4] net/ena: enable WC

[Michał Krawczyk]

2018-06-28 15:15 GMT+02:00 Rafal Kozik <rk@semihalf.com>:
>
> Write combining (WC) increases NIC performance by making better
> utilization of PCI bus. ENA PMD may make usage of this feature.
>
> To enable it load igb_uio driver with wc_activate set to 1.
>
> Signed-off-by: Rafal Kozik <rk@semihalf.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Michal Krawczyk <mk@semihalf.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 9ae73e3..1870edf 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -2210,7 +2210,8 @@ static int eth_ena_pci_remove(struct rte_pci_device *pci_dev)
>
>  static struct rte_pci_driver rte_ena_pmd = {
>         .id_table = pci_id_ena_map,
> -       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
> +                    RTE_PCI_DRV_WC_ACTIVATE,
>         .probe = eth_ena_pci_probe,
>         .remove = eth_ena_pci_remove,
>  };
> --
> 2.7.4
>