[v4,1/4] igb_uio: add wc option
diff mbox series

Message ID 1530274637-10156-2-git-send-email-rk@semihalf.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • Support for write combining.
Related show

Checks

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

Commit Message

Rafal Kozik June 29, 2018, 12:17 p.m. UTC
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(-)

Comments

Rafal Kozik June 29, 2018, 1:11 p.m. UTC | #1
> 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
>
Ferruh Yigit June 29, 2018, 1:20 p.m. UTC | #2
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)
Ferruh Yigit June 29, 2018, 1:40 p.m. UTC | #3
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
diff mbox series

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");