All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1]  Add config option for batched hcalls
@ 2010-09-24 21:44 Will Schmidt
  2010-09-26  3:49 ` Olof Johansson
  0 siblings, 1 reply; 6+ messages in thread
From: Will Schmidt @ 2010-09-24 21:44 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Anton Blanchard


Add a config option for the (batched) MULTITCE and BULK_REMOVE h-calls.

By default, these options are on and are beneficial for performance and
throughput reasons.   If disabled, the code will fall back to using less
optimal TCE and REMOVE hcalls.   The ability to easily disable these
options is useful for some of the PREEMPT_RT related investigation and
work occurring on Power.


Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
cc: Anton Blanchard <anton@samba.org>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---
diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
index f0e6f28..0b5e6a9 100644
--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig
@@ -81,3 +81,23 @@ config DTL
 	  which are accessible through a debugfs file.
 
 	  Say N if you are unsure.
+
+config BULK_REMOVE
+	bool "Enable BULK_REMOVE"
+	depends on PPC_PSERIES
+	default y
+	help
+	  Enable the BULK_REMOVE option for the hash page code.
+	  This relies on a "hcall-bulk" firmware feature, and
+	  should be enabled for performance throughput.
+
+config MULTITCE
+	bool "Enable MultiTCE"
+	depends on PPC_PSERIES
+	default y
+	help
+	  Enable the Multi-TCE code, allowing a single hcall to
+	  update multiple TCE entries at one time.  This relies
+	  on a "hcall-multi-tce" firmware feature, and should be
+	  enabled for performance throughput.
+
diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
index 0a4d8c..4327064 100644
--- a/arch/powerpc/platforms/pseries/firmware.c
+++ b/arch/powerpc/platforms/pseries/firmware.c
@@ -51,9 +51,13 @@ firmware_features_table[FIRMWARE_MAX_FEATURES] = {
 	{FW_FEATURE_VIO,		"hcall-vio"},
 	{FW_FEATURE_RDMA,		"hcall-rdma"},
 	{FW_FEATURE_LLAN,		"hcall-lLAN"},
+#if defined(CONFIG_BULK_REMOVE)
 	{FW_FEATURE_BULK_REMOVE,	"hcall-bulk"},
+#endif
 	{FW_FEATURE_XDABR,		"hcall-xdabr"},
+#if defined(CONFIG_MULTITCE)
 	{FW_FEATURE_MULTITCE,		"hcall-multi-tce"},
+#endif
 	{FW_FEATURE_SPLPAR,		"hcall-splpar"},
 };
 
 /* Build up the firmware features bitmask using the contents of

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1]  Add config option for batched hcalls
  2010-09-24 21:44 [PATCH 1/1] Add config option for batched hcalls Will Schmidt
@ 2010-09-26  3:49 ` Olof Johansson
  2010-09-27 20:06   ` Will Schmidt
  2010-09-28 17:02   ` [PATCH 1/1 v2 ] Add kernel parameter to disable " Will Schmidt
  0 siblings, 2 replies; 6+ messages in thread
From: Olof Johansson @ 2010-09-26  3:49 UTC (permalink / raw)
  To: Will Schmidt; +Cc: linuxppc-dev, Anton Blanchard

On Fri, Sep 24, 2010 at 04:44:15PM -0500, Will Schmidt wrote:
> 
> Add a config option for the (batched) MULTITCE and BULK_REMOVE h-calls.
> 
> By default, these options are on and are beneficial for performance and
> throughput reasons.   If disabled, the code will fall back to using less
> optimal TCE and REMOVE hcalls.   The ability to easily disable these
> options is useful for some of the PREEMPT_RT related investigation and
> work occurring on Power.

Hi,

I can see why it's useful to enable and disable, but these are all
runtime-checked, wouldn't it be more useful to add a bootarg to handle
it instead of adding some new config options that pretty much everyone
will always go with the defaults on?

The bits are set early, but from looking at where they're used, there
doesn't seem to be any harm in disabling them later on when a bootarg
is convenient to parse and deal with?

It has the benefit of easier on/off testing, if that has any value for
production debug down the road.


-Olof

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1]  Add config option for batched hcalls
  2010-09-26  3:49 ` Olof Johansson
@ 2010-09-27 20:06   ` Will Schmidt
  2010-09-28 17:02   ` [PATCH 1/1 v2 ] Add kernel parameter to disable " Will Schmidt
  1 sibling, 0 replies; 6+ messages in thread
From: Will Schmidt @ 2010-09-27 20:06 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev, Anton Blanchard

On Sat, 2010-09-25 at 22:49 -0500, Olof Johansson wrote:
> On Fri, Sep 24, 2010 at 04:44:15PM -0500, Will Schmidt wrote:
> > 
> > Add a config option for the (batched) MULTITCE and BULK_REMOVE h-calls.
> > 
> > By default, these options are on and are beneficial for performance and
> > throughput reasons.   If disabled, the code will fall back to using less
> > optimal TCE and REMOVE hcalls.   The ability to easily disable these
> > options is useful for some of the PREEMPT_RT related investigation and
> > work occurring on Power.
> 
> Hi,
> 
> I can see why it's useful to enable and disable, but these are all
> runtime-checked, wouldn't it be more useful to add a bootarg to handle
> it instead of adding some new config options that pretty much everyone
> will always go with the defaults on?
> 
> The bits are set early, but from looking at where they're used, there
> doesn't seem to be any harm in disabling them later on when a bootarg
> is convenient to parse and deal with?
> 
> It has the benefit of easier on/off testing, if that has any value for
> production debug down the road.

Hi Olof, 
  Thats a good idea, let me poke at this a bit more, see if I can get
bootargs for this.  

Thanks, 
-Will

> 
> 
> -Olof
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/1  v2 ]  Add kernel parameter to disable batched hcalls
  2010-09-26  3:49 ` Olof Johansson
  2010-09-27 20:06   ` Will Schmidt
@ 2010-09-28 17:02   ` Will Schmidt
  2010-09-28 17:52     ` Olof Johansson
  1 sibling, 1 reply; 6+ messages in thread
From: Will Schmidt @ 2010-09-28 17:02 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev, Anton Blanchard


This introduces a pair of kernel parameters that can be used to disable
the MULTITCE and BULK_REMOVE h-calls. 

By default, those hcalls are enabled, active, and good for throughput
and performance.  The ability to disable them will be useful for some of
the PREEMPT_RT related investigation and work occurring on Power.


Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
cc: Olof Johansson <olof@lixom.net>
cc: Anton Blanchard <anton@samba.org>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---

v2 - Per feedback from Olof, the code is reworked to utilize kernel
parameter runtime checks, rather than CONFIG options.
   - Added relevant change to kernel-parameters.txt



diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e2c7487..5c40801 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -426,6 +426,10 @@ and is between 256 and 4096 characters. It is defined in the file
 	bttv.pll=	See Documentation/video4linux/bttv/Insmod-options
 	bttv.tuner=	and Documentation/video4linux/bttv/CARDLIST
 
+	bulk_remove=off	[PPC]  This parameter disables the use of the pSeries
+			firmware feature for flushing multiple hpte entries
+			at a time.
+
 	BusLogic=	[HW,SCSI]
 			See drivers/scsi/BusLogic.c, comment before function
 			BusLogic_ParseDriverOptions().
@@ -1499,6 +1503,10 @@ and is between 256 and 4096 characters. It is defined in the file
 	mtdparts=	[MTD]
 			See drivers/mtd/cmdlinepart.c.
 
+	multitce=off	[PPC]  This parameter disables the use of the pSeries
+			firmware feature for updating multiple TCE entries
+			at a time.
+
 	onenand.bdry=	[HW,MTD] Flex-OneNAND Boundary Configuration
 
 			Format: [die0_boundary][,die0_lock][,die1_boundary][,die1_lock]
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 902987d..e174a2f 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -625,3 +625,19 @@ void iommu_init_early_pSeries(void)
 	set_pci_dma_ops(&dma_iommu_ops);
 }
 
+static int __init disable_multitce(char *str)
+{
+	if (strcmp(str,"off")==0) {
+		if (firmware_has_feature(FW_FEATURE_LPAR)) {
+			if (firmware_has_feature(FW_FEATURE_MULTITCE)) {
+				printk(KERN_INFO "Disabling MULTITCE firmware feature\n");
+				ppc_md.tce_build = tce_build_pSeriesLP;
+				ppc_md.tce_free	 = tce_free_pSeriesLP;
+				powerpc_firmware_features &= ~FW_FEATURE_MULTITCE;
+			}
+		}
+	}
+	return 1;
+}
+
+__setup("multitce=",disable_multitce);
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 0707653..82d15e7 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -599,6 +599,19 @@ static void pSeries_lpar_flush_hash_range(unsigned long number, int local)
 		spin_unlock_irqrestore(&pSeries_lpar_tlbie_lock, flags);
 }
 
+static int __init disable_bulk_remove(char *str)
+{
+	if (strcmp(str,"off")==0) {
+		if (firmware_has_feature(FW_FEATURE_BULK_REMOVE)) {
+			printk(KERN_INFO "Disabling BULK_REMOVE firmware feature");
+			powerpc_firmware_features &= ~FW_FEATURE_BULK_REMOVE;
+		}
+	}
+	return 1;
+}
+
+__setup("bulk_remove=",disable_bulk_remove);
+
 void __init hpte_init_lpar(void)
 {
 	ppc_md.hpte_invalidate	= pSeries_lpar_hpte_invalidate;

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1  v2 ]  Add kernel parameter to disable batched hcalls
  2010-09-28 17:02   ` [PATCH 1/1 v2 ] Add kernel parameter to disable " Will Schmidt
@ 2010-09-28 17:52     ` Olof Johansson
  2010-09-29  1:33       ` [PATCH 1/1 v3] " Will Schmidt
  0 siblings, 1 reply; 6+ messages in thread
From: Olof Johansson @ 2010-09-28 17:52 UTC (permalink / raw)
  To: Will Schmidt; +Cc: linuxppc-dev, Anton Blanchard

Nice. I've got minor nits below, and you might also want to run the patch
through checkpatch and fix up some of the whitespace warnings.


-Olof

On Tue, Sep 28, 2010 at 12:02:51PM -0500, Will Schmidt wrote:
> 
> This introduces a pair of kernel parameters that can be used to disable
> the MULTITCE and BULK_REMOVE h-calls. 
> 
> By default, those hcalls are enabled, active, and good for throughput
> and performance.  The ability to disable them will be useful for some of
> the PREEMPT_RT related investigation and work occurring on Power.
> 
> 
> Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
> cc: Olof Johansson <olof@lixom.net>
> cc: Anton Blanchard <anton@samba.org>
> cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> ---
> 
> v2 - Per feedback from Olof, the code is reworked to utilize kernel
> parameter runtime checks, rather than CONFIG options.
>    - Added relevant change to kernel-parameters.txt
> 
> 
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index e2c7487..5c40801 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -426,6 +426,10 @@ and is between 256 and 4096 characters. It is defined in the file
>  	bttv.pll=	See Documentation/video4linux/bttv/Insmod-options
>  	bttv.tuner=	and Documentation/video4linux/bttv/CARDLIST
>  
> +	bulk_remove=off	[PPC]  This parameter disables the use of the pSeries
> +			firmware feature for flushing multiple hpte entries
> +			at a time.
> +
>  	BusLogic=	[HW,SCSI]
>  			See drivers/scsi/BusLogic.c, comment before function
>  			BusLogic_ParseDriverOptions().
> @@ -1499,6 +1503,10 @@ and is between 256 and 4096 characters. It is defined in the file
>  	mtdparts=	[MTD]
>  			See drivers/mtd/cmdlinepart.c.
>  
> +	multitce=off	[PPC]  This parameter disables the use of the pSeries
> +			firmware feature for updating multiple TCE entries
> +			at a time.
> +
>  	onenand.bdry=	[HW,MTD] Flex-OneNAND Boundary Configuration
>  
>  			Format: [die0_boundary][,die0_lock][,die1_boundary][,die1_lock]
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 902987d..e174a2f 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -625,3 +625,19 @@ void iommu_init_early_pSeries(void)
>  	set_pci_dma_ops(&dma_iommu_ops);
>  }
>  
> +static int __init disable_multitce(char *str)
> +{
> +	if (strcmp(str,"off")==0) {
> +		if (firmware_has_feature(FW_FEATURE_LPAR)) {
> +			if (firmware_has_feature(FW_FEATURE_MULTITCE)) {
> +				printk(KERN_INFO "Disabling MULTITCE firmware feature\n");
> +				ppc_md.tce_build = tce_build_pSeriesLP;
> +				ppc_md.tce_free	 = tce_free_pSeriesLP;
> +				powerpc_firmware_features &= ~FW_FEATURE_MULTITCE;
> +			}
> +		}
> +	}

I personally prefer to keep cases like these in one if statement to save indentation:

	if (strcmp(str, "off") == 0 &&
	    firmware_has_feature(FW_FEATURE_LPAR) &&
	    firmware_has_feature(FW_FEATURE_MULTITCE)) {
		<...>
	}

> +	return 1;
> +}
> +
> +__setup("multitce=",disable_multitce);
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index 0707653..82d15e7 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -599,6 +599,19 @@ static void pSeries_lpar_flush_hash_range(unsigned long number, int local)
>  		spin_unlock_irqrestore(&pSeries_lpar_tlbie_lock, flags);
>  }
>  
> +static int __init disable_bulk_remove(char *str)
> +{
> +	if (strcmp(str,"off")==0) {
> +		if (firmware_has_feature(FW_FEATURE_BULK_REMOVE)) {
> +			printk(KERN_INFO "Disabling BULK_REMOVE firmware feature");
> +			powerpc_firmware_features &= ~FW_FEATURE_BULK_REMOVE;
> +		}
> +	}

Same here.

> +	return 1;
> +}
> +
> +__setup("bulk_remove=",disable_bulk_remove);
> +
>  void __init hpte_init_lpar(void)
>  {
>  	ppc_md.hpte_invalidate	= pSeries_lpar_hpte_invalidate;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1  v3]  Add kernel parameter to disable batched hcalls
  2010-09-28 17:52     ` Olof Johansson
@ 2010-09-29  1:33       ` Will Schmidt
  0 siblings, 0 replies; 6+ messages in thread
From: Will Schmidt @ 2010-09-29  1:33 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev, Anton Blanchard


This introduces a pair of kernel parameters that can be used to disable
the MULTITCE and BULK_REMOVE h-calls.

By default, those hcalls are enabled, active, and good for throughput
and performance.  The ability to disable them will be useful for some of
the PREEMPT_RT related investigation and work occurring on Power.

Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
cc: Olof Johansson <olof@lixom.net>
cc: Anton Blanchard <anton@samba.org>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---

v2 - Per feedback from Olof, the code is reworked to utilize kernel
    parameter runtime checks, rather than CONFIG options.
   - Added relevant change to kernel-parameters.txt

v3 - ran through checkpatch and fixed whitespace issues.
   - consolidated the if() staircases to save indentation.


diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e2c7487..5c40801 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -426,6 +426,10 @@ and is between 256 and 4096 characters. It is defined in the file
 	bttv.pll=	See Documentation/video4linux/bttv/Insmod-options
 	bttv.tuner=	and Documentation/video4linux/bttv/CARDLIST
 
+	bulk_remove=off	[PPC]  This parameter disables the use of the pSeries
+			firmware feature for flushing multiple hpte entries
+			at a time.
+
 	BusLogic=	[HW,SCSI]
 			See drivers/scsi/BusLogic.c, comment before function
 			BusLogic_ParseDriverOptions().
@@ -1499,6 +1503,10 @@ and is between 256 and 4096 characters. It is defined in the file
 	mtdparts=	[MTD]
 			See drivers/mtd/cmdlinepart.c.
 
+	multitce=off	[PPC]  This parameter disables the use of the pSeries
+			firmware feature for updating multiple TCE entries
+			at a time.
+
 	onenand.bdry=	[HW,MTD] Flex-OneNAND Boundary Configuration
 
 			Format: [die0_boundary][,die0_lock][,die1_boundary][,die1_lock]
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 902987d..e174a2f 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -625,3 +625,17 @@ void iommu_init_early_pSeries(void)
 	set_pci_dma_ops(&dma_iommu_ops);
 }
 
+static int __init disable_multitce(char *str)
+{
+	if (strcmp(str, "off") == 0 &&
+	    firmware_has_feature(FW_FEATURE_LPAR) &&
+	    firmware_has_feature(FW_FEATURE_MULTITCE)) {
+		printk(KERN_INFO "Disabling MULTITCE firmware feature\n");
+		ppc_md.tce_build = tce_build_pSeriesLP;
+		ppc_md.tce_free	 = tce_free_pSeriesLP;
+		powerpc_firmware_features &= ~FW_FEATURE_MULTITCE;
+	}
+	return 1;
+}
+
+__setup("multitce=", disable_multitce);
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 0707653..82d15e7 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -599,6 +599,18 @@ static void pSeries_lpar_flush_hash_range(unsigned long number, int local)
 		spin_unlock_irqrestore(&pSeries_lpar_tlbie_lock, flags);
 }
 
+static int __init disable_bulk_remove(char *str)
+{
+	if (strcmp(str, "off") == 0 &&
+	    firmware_has_feature(FW_FEATURE_BULK_REMOVE)) {
+			printk(KERN_INFO "Disabling BULK_REMOVE firmware feature");
+			powerpc_firmware_features &= ~FW_FEATURE_BULK_REMOVE;
+	}
+	return 1;
+}
+
+__setup("bulk_remove=", disable_bulk_remove);
+
 void __init hpte_init_lpar(void)
 {
 	ppc_md.hpte_invalidate	= pSeries_lpar_hpte_invalidate;

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-29  1:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-24 21:44 [PATCH 1/1] Add config option for batched hcalls Will Schmidt
2010-09-26  3:49 ` Olof Johansson
2010-09-27 20:06   ` Will Schmidt
2010-09-28 17:02   ` [PATCH 1/1 v2 ] Add kernel parameter to disable " Will Schmidt
2010-09-28 17:52     ` Olof Johansson
2010-09-29  1:33       ` [PATCH 1/1 v3] " Will Schmidt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.