New CMC/CPE polling

PATCH In review Alex Williamson 4 participants 10 messages Jul 31, 2003 – Aug 8, 2003

Share & tools

Alex Williamson

author

2003-07-31 23:06 UTC #0 lore raw

PATCH 1/1 New CMC/CPE polling

Here's a redesign of the CMC and CPE polling for both 2.6.0-test2
and 2.4.21. This is roughly the same design I requested comment on a while back (BTW, nobody commented...). Basically, rather than flooding all the cpus in parallel, I used some low priority interrupts to cascade through the cpus. This should be much more scalable. I also added a new feature of enabling interrupts for the CMC and CPE handlers. The SAL spec claims these functions are SMP safe and re-entrant and even recommends that the corrected error handlers should run with interrupts enabled. It works on HP boxes, others might want to double check that their firmware adheres to the spec. The combination of these things should keep polling from impacting system response time. I tried to keep the 2.6 and 2.4 code as similar as possible, so I also backported __ffs() to 2.4. Feedback and bug reports welcome. Thanks,

Alex

Signature · 1 line

Alex Williamson                             HP Linux & Open Source Lab

arch/ia64/kernel/iosapic.c +1 -1

@@ -543,7 +543,7 @@
 		delivery = IOSAPIC_INIT;
 		break;
 	      case ACPI_INTERRUPT_CPEI:
-		vector = IA64_PCE_VECTOR;
+		vector = IA64_CPE_VECTOR;
 		delivery = IOSAPIC_LOWEST_PRIORITY;
 		break;
 	      default:

arch/ia64/sn/kernel/setup.c +1 -1

@@ -280,7 +280,7 @@
 	else
 		sn_rtc_cycles_per_second = ticks_per_sec;
 
-	platform_intr_list[ACPI_INTERRUPT_CPEI] = IA64_PCE_VECTOR;
+	platform_intr_list[ACPI_INTERRUPT_CPEI] = IA64_CPE_VECTOR;
 
 
 	if ( IS_RUNNING_ON_SIMULATOR() )

include/asm-ia64/mca.h +2 -0

@@ -137,7 +137,9 @@
 extern irqreturn_t ia64_mca_rendez_int_handler(int,void *,struct pt_regs *);
 extern irqreturn_t ia64_mca_wakeup_int_handler(int,void *,struct pt_regs *);
 extern irqreturn_t ia64_mca_cmc_int_handler(int,void *,struct pt_regs *);
+extern irqreturn_t ia64_mca_cmc_int_caller(int,void *,struct pt_regs *);
 extern irqreturn_t ia64_mca_cpe_int_handler(int,void *,struct pt_regs *);
+extern irqreturn_t ia64_mca_cpe_int_caller(int,void *,struct pt_regs *);
 extern int  ia64_log_print(int,prfunc_t);
 extern void ia64_mca_cmc_vector_setup(void);
 extern int  ia64_mca_check_errors(void);

include/asm-ia64/hw_irq.h +3 -1

@@ -38,7 +38,9 @@
 /*
  * Vectors 0x10-0x1f are used for low priority interrupts, e.g. CMCI.
  */
-#define IA64_PCE_VECTOR			0x1e	/* platform corrected error interrupt vector */
+#define IA64_CPEP_VECTOR		0x1c	/* corrected platform error polling vector */
+#define IA64_CMCP_VECTOR		0x1d	/* correctable machine-check polling vector */
+#define IA64_CPE_VECTOR			0x1e	/* corrected platform error interrupt vector */
 #define IA64_CMC_VECTOR			0x1f	/* correctable machine-check interrupt vector */
 /*
  * Vectors 0x20-0x2f are reserved for legacy ISA IRQs.

include/asm-ia64/sal.h +13 -6

@@ -68,6 +68,13 @@
 	ia64_load_scratch_fpregs(__ia64_scn_fr);	\
 } while (0)
 
+# define SAL_CALL_SAFE(result,args...) do {		\
+	struct ia64_fpreg __ia64_scs_fr[6];		\
+	ia64_save_scratch_fpregs(__ia64_scs_fr);	\
+	__SAL_CALL(result, args);			\
+	ia64_load_scratch_fpregs(__ia64_scs_fr);	\
+} while (0)
+
 #define SAL_SET_VECTORS			0x01000000
 #define SAL_GET_STATE_INFO		0x01000001
 #define SAL_GET_STATE_INFO_SIZE		0x01000002

@@ -665,8 +672,8 @@
 ia64_sal_clear_state_info (u64 sal_info_type)
 {
 	struct ia64_sal_retval isrv;
-    SAL_CALL(isrv, SAL_CLEAR_STATE_INFO, sal_info_type, 0,
-             0, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_CLEAR_STATE_INFO, sal_info_type, 0,
+	              0, 0, 0, 0, 0);
 	return isrv.status;
 }

@@ -678,8 +685,8 @@
 ia64_sal_get_state_info (u64 sal_info_type, u64 *sal_info)
 {
 	struct ia64_sal_retval isrv;
-	SAL_CALL(isrv, SAL_GET_STATE_INFO, sal_info_type, 0,
-	         sal_info, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_GET_STATE_INFO, sal_info_type, 0,
+	              sal_info, 0, 0, 0, 0);
 	if (isrv.status)
 		return 0;

@@ -694,8 +701,8 @@
 ia64_sal_get_state_info_size (u64 sal_info_type)
 {
 	struct ia64_sal_retval isrv;
-    SAL_CALL(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type, 0,
-             0, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type, 0,
+	              0, 0, 0, 0, 0);
 	if (isrv.status)
 		return 0;
 	return isrv.v0;

arch/ia64/kernel/mca.c +145 ~~-77~~

328 lines not on this page · raw message

arch/ia64/kernel/iosapic.c +1 -1

@@ -530,7 +530,7 @@
 		delivery = IOSAPIC_INIT;
 		break;
 	      case ACPI_INTERRUPT_CPEI:
-		vector = IA64_PCE_VECTOR;
+		vector = IA64_CPE_VECTOR;
 		delivery = IOSAPIC_LOWEST_PRIORITY;
 		break;
 	      default:

arch/ia64/sn/kernel/setup.c +1 -1

@@ -277,7 +277,7 @@
 	else
 		sn_rtc_cycles_per_second = ticks_per_sec;
 
-	platform_intr_list[ACPI_INTERRUPT_CPEI] = IA64_PCE_VECTOR;
+	platform_intr_list[ACPI_INTERRUPT_CPEI] = IA64_CPE_VECTOR;
 
 
 	if ( IS_RUNNING_ON_SIMULATOR() )

include/asm-ia64/mca.h +2 -0

@@ -132,7 +132,9 @@
 extern void ia64_mca_rendez_int_handler(int,void *,struct pt_regs *);
 extern void ia64_mca_wakeup_int_handler(int,void *,struct pt_regs *);
 extern void ia64_mca_cmc_int_handler(int,void *,struct pt_regs *);
+extern void ia64_mca_cmc_int_caller(int,void *,struct pt_regs *);
 extern void ia64_mca_cpe_int_handler(int,void *,struct pt_regs *);
+extern void ia64_mca_cpe_int_caller(int,void *,struct pt_regs *);
 extern int  ia64_log_print(int,prfunc_t);
 extern void ia64_mca_cmc_vector_setup(void);
 extern int  ia64_mca_check_errors(void);

include/asm-ia64/hw_irq.h +3 -1

@@ -37,7 +37,9 @@
 /*
  * Vectors 0x10-0x1f are used for low priority interrupts, e.g. CMCI.
  */
-#define IA64_PCE_VECTOR			0x1e	/* platform corrected error interrupt vector */
+#define IA64_CPEP_VECTOR		0x1c	/* corrected platform error polling vector */
+#define IA64_CMCP_VECTOR		0x1d	/* correctable machine-check polling vector */
+#define IA64_CPE_VECTOR			0x1e	/* corrected platform error interrupt vector */
 #define IA64_CMC_VECTOR			0x1f	/* correctable machine-check interrupt vector */
 /*
  * Vectors 0x20-0x2f are reserved for legacy ISA IRQs.

include/asm-ia64/sal.h +13 -6

@@ -56,6 +56,13 @@
 	ia64_load_scratch_fpregs(__ia64_scn_fr);	\
 } while (0)
 
+# define SAL_CALL_SAFE(result,args...) do {		\
+	struct ia64_fpreg __ia64_scs_fr[6];		\
+	ia64_save_scratch_fpregs(__ia64_scs_fr);	\
+	__SAL_CALL(result, args);			\
+	ia64_load_scratch_fpregs(__ia64_scs_fr);	\
+} while (0)
+
 #define SAL_SET_VECTORS			0x01000000
 #define SAL_GET_STATE_INFO		0x01000001
 #define SAL_GET_STATE_INFO_SIZE		0x01000002

@@ -658,8 +665,8 @@
 ia64_sal_clear_state_info (u64 sal_info_type)
 {
 	struct ia64_sal_retval isrv;
-    SAL_CALL(isrv, SAL_CLEAR_STATE_INFO, sal_info_type, 0,
-             0, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_CLEAR_STATE_INFO, sal_info_type, 0,
+	              0, 0, 0, 0, 0);
 	return isrv.status;
 }

@@ -671,8 +678,8 @@
 ia64_sal_get_state_info (u64 sal_info_type, u64 *sal_info)
 {
 	struct ia64_sal_retval isrv;
-	SAL_CALL(isrv, SAL_GET_STATE_INFO, sal_info_type, 0,
-	         sal_info, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_GET_STATE_INFO, sal_info_type, 0,
+	              sal_info, 0, 0, 0, 0);
 	if (isrv.status)
 		return 0;

@@ -687,8 +694,8 @@
 ia64_sal_get_state_info_size (u64 sal_info_type)
 {
 	struct ia64_sal_retval isrv;
-    SAL_CALL(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type, 0,
-             0, 0, 0, 0, 0);
+	SAL_CALL_SAFE(isrv, SAL_GET_STATE_INFO_SIZE, sal_info_type, 0,
+	              0, 0, 0, 0, 0);
 	if (isrv.status)
 		return 0;
 	return isrv.v0;

arch/ia64/kernel/mca.c +141 ~~-78~~

323 lines not on this page · raw message

include/asm-ia64/bitops.h +15 -0

@@ -282,6 +282,21 @@
 	return result;
 }
 
+/**
+ * __ffs - find first bit in word.
+ * @x: The word to search
+ *
+ * Undefined if no bit exists, so code should check against 0 first.
+ */
+static __inline__ unsigned long
+__ffs (unsigned long x)
+{
+	unsigned long result;
+
+	__asm__ ("popcnt %0=%1" : "=r" (result) : "r" ((x - 1) & ~x));
+	return result;
+}
+
 #ifdef __KERNEL__
 
 /*

David Mosberger

2003-08-01 06:34 UTC #1 lore raw

Alex> +# define SAL_CALL_SAFE(result,args...)

How about calling this SAL_CALL_REENTRANT() or something like that? SAFE isn't particularly meaningful and it's backwards from the caller's point of view (where SAL_CALL() is the safe macro to use...).

	--david

Alex Williamson

author

2003-08-01 14:20 UTC #2 lore raw

In reply to Alex Williamson

Quoted 6 lines · David Mosberger

Alex> +# define SAL_CALL_SAFE(result,args...)
How about calling this SAL_CALL_REENTRANT() or something like that? SAFE isn't particularly meaningful and it's backwards from the caller's point of view (where SAL_CALL() is the safe macro to use...).

Perhaps SAL_CALL_DANGEROUS() ;^) Yes, I agree, the name is misleading
from the caller perspective. SAL_CALL_REENTRANT() is a much better choice. Let me know if you want an updated patch. Thanks,

Alex

Signature · 1 line

Alex Williamson                             HP Linux & Open Source Lab

Luck, Tony

2003-08-04 18:26 UTC #3 lore raw

In reply to Alex Williamson

Quoted 6 lines

Here's a redesign of the CMC and CPE polling for both 2.6.0-test2
and 2.4.21. This is roughly the same design I requested comment on a while back (BTW, nobody commented...). Basically, rather than flooding all the cpus in parallel, I used some low priority interrupts to cascade through the cpus. This should be much more scalable. I also added a new feature of enabling interrupts for the CMC and CPE

Quoted 8 lines

handlers. The SAL spec claims these functions are SMP safe and re-entrant and even recommends that the corrected error handlers should run with interrupts enabled. It works on HP boxes, others might want to double check that their firmware adheres to the spec. The combination of these things should keep polling from impacting system response time. I tried to keep the 2.6 and 2.4 code as similar as possible, so I also backported __ffs() to 2.4. Feedback and bug reports welcome. Thanks,

I think that there are some race conditions around the setting/clearing of cmc_polling_enabled.

-Tony

Alex Williamson

author

2003-08-04 18:49 UTC #4 lore raw

In reply to Alex Williamson

Quoted 4 lines · Tony

I think that there are some race conditions around the setting/clearing of cmc_polling_enabled.

Tony,

You might be right on the clearing side, I think moving it down
a couple lines and disabling local interrupts would eliminate the potential hole though. Something like this:

ia64_mca_cmc_int_caller(...)
{
...

    smp_call_function(ia64_mca_cmc_vector_enable, NULL, 1, 0);
    local_irq_disable();
    ia64_mca_cmc_vector_enable(NULL);
    cmc_polling_enabled = 0;

...

Does that address the race you were looking at? I don't see one
on the setting end, could you be more specific? The spinlock feels like it does the trick to me. Thanks for the comments,

Alex

Signature · 1 line

Alex Williamson                             HP Linux & Open Source Lab

Luck, Tony

2003-08-04 20:43 UTC #5 lore raw

In reply to Alex Williamson

Quoted 11 lines

You might be right on the clearing side, I think moving it down
a couple lines and disabling local interrupts would eliminate the potential hole though. Something like this:
ia64_mca_cmc_int_caller(...)
{
...
    smp_call_function(ia64_mca_cmc_vector_enable, NULL, 1, 0);
    local_irq_disable();
    ia64_mca_cmc_vector_enable(NULL);
    cmc_polling_enabled = 0;

Quoted 5 lines

...
Does that address the race you were looking at? I don't see one
on the setting end, could you be more specific? The spinlock feels like it does the trick to me. Thanks for the comments,

My issue is that you check and set cmc_polling_enabled under the protection of a lock (cmc_history_lock ... I realise that this is mostly protecting the cmc_history[], but it also defends against multiple cpus taking a CMCI at the same time and all trying to switch over to polling mode).

But you clear cmc_polling_enabled without the lock. So the race is between enabling the interrupt (on all cpus) and taking the next interrupt. The local_irq_disable() in the above fragment fixes that for the current cpu, but for other cpus you can end up in the handler with cmc_polling_enabled set to the wrong value.

One way might be harmless, but I'm not sure which.

-Tony

Alex Williamson

author

2003-08-04 21:15 UTC #6 lore raw

In reply to Alex Williamson

Quoted 24 lines · Tony

Quoted 12 lines
You might be right on the clearing side, I think moving it down
a couple lines and disabling local interrupts would eliminate the potential hole though. Something like this:
ia64_mca_cmc_int_caller(...)
{
...
    smp_call_function(ia64_mca_cmc_vector_enable, NULL, 1, 0);
    local_irq_disable();
    ia64_mca_cmc_vector_enable(NULL);
    cmc_polling_enabled = 0;
Quoted 5 lines
...
Does that address the race you were looking at? I don't see one
on the setting end, could you be more specific? The spinlock feels like it does the trick to me. Thanks for the comments,
My issue is that you check and set cmc_polling_enabled under the protection of a lock (cmc_history_lock ... I realise that this is mostly protecting the cmc_history[], but it also defends against multiple cpus taking a CMCI at the same time and all trying to switch over to polling mode).

Quoted 7 lines

But you clear cmc_polling_enabled without the lock. So the race is between enabling the interrupt (on all cpus) and taking the next interrupt. The local_irq_disable() in the above fragment fixes that for the current cpu, but for other cpus you can end up in the handler with cmc_polling_enabled set to the wrong value.
One way might be harmless, but I'm not sure which.

Tony,

I believe it is harmless. I make use of the spinlock when setting
cmc_polling_enabled to serialize the cpus. ia64_mca_cmc_int_handler() can obviously have multiple CPUs in it at the same time. However, the polling sequence is naturally serialized, so there will never be more than one CPU in the block that clears cmc_polling_enabled. I didn't feel it necessary to pull the spinlock out of the interrupt handler when it wasn't needed.

With the modifications above, there is a tiny window where the
other CPUs could take a CMCI while the cmc_polling_enabled flag is in the wrong state. IMHO, this doesn't really seem like a problem. The goal of polling for CMCs is simply to make the system usable, not to count every CMC that occurs. Forward progress towards setting the flag in the right state is ensured by disabling interrupts, so the window is guaranteed to be very small. FWIW, this is the same state we're in between ia64_mca_init() and ia64_mca_late_init().

Alex

Signature · 1 line

Alex Williamson                             HP Linux & Open Source Lab

Luck, Tony

2003-08-04 21:53 UTC #7 lore raw

In reply to Alex Williamson

Quoted 3 lines

With the modifications above, there is a tiny window where the
other CPUs could take a CMCI while the cmc_polling_enabled flag is in the wrong state. IMHO, this doesn't really seem like a problem.

Yup, you are right.

-Tony

David Mosberger

2003-08-04 23:09 UTC #8 lore raw

In reply to Alex Williamson

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Alex Williamson

Alex> Perhaps SAL_CALL_DANGEROUS() ;^) Yes, I agree, the name is misleading Alex> from the caller perspective. SAL_CALL_REENTRANT() is a much better Alex> choice. Let me know if you want an updated patch. Thanks,

Nah, that's OK, I just edited the patch.

	--david

Bjorn Helgaas

2003-08-08 18:39 UTC #9 lore raw

In reply to Alex Williamson

Quoted 6 lines · Alex Williamson

Here's a redesign of the CMC and CPE polling for both 2.6.0-test2
and 2.4.21. This is roughly the same design I requested comment on a while back (BTW, nobody commented...). Basically, rather than flooding all the cpus in parallel, I used some low priority interrupts to cascade through the cpus. This should be much more scalable. I also added a new feature of enabling interrupts for the CMC and CPE

Quoted 8 lines

handlers. The SAL spec claims these functions are SMP safe and re-entrant and even recommends that the corrected error handlers should run with interrupts enabled. It works on HP boxes, others might want to double check that their firmware adheres to the spec. The combination of these things should keep polling from impacting system response time. I tried to keep the 2.6 and 2.4 code as similar as possible, so I also backported __ffs() to 2.4. Feedback and bug reports welcome. Thanks,

I applied this for 2.4. I also applied the following patch to fix some old comments that were slightly misleading (CMC's and CPE's have already been "corrected"; they're not "correctable".)

#### AUTHOR [email protected]
#### COMMENT START
### Comments for ChangeSet
ia64: Comment changes to fix "correctable" usage.
### Comments for arch/ia64/kernel/acpi.c
Fix usage ("corrected" machine checks and platform errors, not "correctable"). ### Comments for include/asm-ia64/hw_irq.h
Fix usage ("corrected" machine checks and platform errors, not "correctable"). ### Comments for include/asm-ia64/processor.h
Fix usage ("corrected" machine checks and platform errors, not "correctable"). #### COMMENT END

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1075 -> 1.1076
# include/asm-ia64/processor.h 1.22 -> 1.23
# include/asm-ia64/hw_irq.h 1.5 -> 1.6
# arch/ia64/kernel/acpi.c 1.22 -> 1.23
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/08/08 [email protected] 1.1076
# ia64: Comment changes to fix "correctable" usage.
# --------------------------------------------
#

arch/ia64/kernel/acpi.c +1 -1

diff -Nru a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
@@ -217,7 +217,7 @@
 	int vector = -1;
 
 	if (int_type < ACPI_MAX_PLATFORM_INTERRUPTS) {
-		/* correctable platform error interrupt */
+		/* corrected platform error interrupt */
 		vector = platform_intr_list[int_type];
 	} else
 		printk(KERN_ERR "acpi_request_vector(): invalid interrupt type\n");

include/asm-ia64/hw_irq.h +2 -2

diff -Nru a/include/asm-ia64/hw_irq.h b/include/asm-ia64/hw_irq.h
@@ -38,9 +38,9 @@
  * Vectors 0x10-0x1f are used for low priority interrupts, e.g. CMCI.
  */
 #define IA64_CPEP_VECTOR		0x1c	/* corrected platform error polling vector */
-#define IA64_CMCP_VECTOR		0x1d	/* correctable machine-check polling vector */
+#define IA64_CMCP_VECTOR		0x1d	/* corrected machine-check polling vector */
 #define IA64_CPE_VECTOR			0x1e	/* corrected platform error interrupt vector */
-#define IA64_CMC_VECTOR			0x1f	/* correctable machine-check interrupt vector */
+#define IA64_CMC_VECTOR			0x1f	/* corrected machine-check interrupt vector */
 /*
  * Vectors 0x20-0x2f are reserved for legacy ISA IRQs.
  */

include/asm-ia64/processor.h +0 -6

diff -Nru a/include/asm-ia64/processor.h b/include/asm-ia64/processor.h
@@ -770,18 +770,12 @@
 #define init_task	(init_task_union.task)
 #define init_stack	(init_task_union.stack)
 
-/*
- * Set the correctable machine check vector register
- */
 static inline void
 ia64_set_cmcv (__u64 val)
 {
 	asm volatile ("mov cr.cmcv=%0" :: "r"(val) : "memory");
 }
 
-/*
- * Read the correctable machine check vector register
- */
 static inline __u64
 ia64_get_cmcv (void)
 {