Correct a series of errors in the hand-rolled locking for drace_debug.c:

- Use spinlock_enter()/spinlock_exit() to prevent a thread holding a
  debug lock from being preempted to prevent other threads waiting
  on that lock from starvation.

- Handle the possibility of CPU migration in between the fetch of curcpu
  and the call to spinlock_enter() by saving curcpu in a local variable.

- Use memory barriers to prevent reordering of loads and stores of the
  data protected by the lock outside of the critical section

- Eliminate false sharing of the locks by moving them into the structures
  that they protect and aligning them to a cacheline boundary.

- Record the owning thread in the lock to make debugging future problems
  easier.

Reviewed by:	rpaulo (initial version)
MFC after:	2 weeks

1 files changed, 54 insertions, 44 deletions

diff --git a/sys/cddl/dev/dtrace/dtrace_debug.c b/sys/cddl/dev/dtrace/dtrace_debug.c
index 7202fe2..3b29662 100644
--- a/sys/cddl/dev/dtrace/dtrace_debug.c
+++ b/sys/cddl/dev/dtrace/dtrace_debug.c
@@ -33,11 +33,10 @@
 
 #include <machine/atomic.h>
 
-#define	dtrace_cmpset_long	atomic_cmpset_long
-
 #define DTRACE_DEBUG_BUFR_SIZE	(32 * 1024)
 
 struct dtrace_debug_data {
+	uintptr_t lock __aligned(CACHE_LINE_SIZE);
 	char bufr[DTRACE_DEBUG_BUFR_SIZE];
 	char *first;
 	char *last;
@@ -46,20 +45,22 @@ struct dtrace_debug_data {
 
 static char dtrace_debug_bufr[DTRACE_DEBUG_BUFR_SIZE];
 
-static volatile u_long	dtrace_debug_flag[MAXCPU];
-
 static void
 dtrace_debug_lock(int cpu)
 {
-	while (dtrace_cmpset_long(&dtrace_debug_flag[cpu], 0, 1) == 0)
-		/* Loop until the lock is obtained. */
+	 uintptr_t tid;
+
+	tid = (uintptr_t)curthread;
+	spinlock_enter();
+	while (atomic_cmpset_acq_ptr(&dtrace_debug_data[cpu].lock, 0, tid) == 0)		/* Loop until the lock is obtained. */
 		;
 }
 
 static void
 dtrace_debug_unlock(int cpu)
 {
-	dtrace_debug_flag[cpu] = 0;
+	atomic_store_rel_ptr(&dtrace_debug_data[cpu].lock, 0);
+	spinlock_exit();
 }
 
 static void
@@ -151,10 +152,11 @@ dtrace_debug_output(void)
  */
 
 static __inline void
-dtrace_debug__putc(char c)
+dtrace_debug__putc(int cpu, char c)
 {
-	struct dtrace_debug_data *d = &dtrace_debug_data[curcpu];
+	struct dtrace_debug_data *d;
 
+	d = &dtrace_debug_data[cpu];
 	*d->next++ = c;
 
 	if (d->next == d->last)
@@ -172,24 +174,30 @@ dtrace_debug__putc(char c)
 static void __used
 dtrace_debug_putc(char c)
 {
-	dtrace_debug_lock(curcpu);
+	int cpu;
+
+	cpu = curcpu;
+	dtrace_debug_lock(cpu);
 
-	dtrace_debug__putc(c);
+	dtrace_debug__putc(cpu, c);
 
-	dtrace_debug_unlock(curcpu);
+	dtrace_debug_unlock(cpu);
 }
 
 static void __used
 dtrace_debug_puts(const char *s)
 {
-	dtrace_debug_lock(curcpu);
+	int cpu;
+	
+	cpu = curcpu;
+	dtrace_debug_lock(cpu);
 
 	while (*s != '\0')
-		dtrace_debug__putc(*s++);
+		dtrace_debug__putc(cpu, *s++);
 
-	dtrace_debug__putc('\0');
+	dtrace_debug__putc(cpu, '\0');
 
-	dtrace_debug_unlock(curcpu);
+	dtrace_debug_unlock(cpu);
 }
 
 /*
@@ -219,7 +227,7 @@ dtrace_debug_ksprintn(char *nbuf, uintmax_t num, int base, int *lenp, int upper)
 #define MAXNBUF (sizeof(intmax_t) * NBBY + 1)
 
 static void
-dtrace_debug_vprintf(const char *fmt, va_list ap)
+dtrace_debug_vprintf(int cpu, const char *fmt, va_list ap)
 {
 	char nbuf[MAXNBUF];
 	const char *p, *percent, *q;
@@ -243,10 +251,10 @@ dtrace_debug_vprintf(const char *fmt, va_list ap)
 		width = 0;
 		while ((ch = (u_char)*fmt++) != '%' || stop) {
 			if (ch == '\0') {
-				dtrace_debug__putc('\0');
+				dtrace_debug__putc(cpu, '\0');
 				return;
 			}
-			dtrace_debug__putc(ch);
+			dtrace_debug__putc(cpu, ch);
 		}
 		percent = fmt - 1;
 		qflag = 0; lflag = 0; ladjust = 0; sharpflag = 0; neg = 0;
@@ -266,7 +274,7 @@ reswitch:	switch (ch = (u_char)*fmt++) {
 			ladjust = 1;
 			goto reswitch;
 		case '%':
-			dtrace_debug__putc(ch);
+			dtrace_debug__putc(cpu, ch);
 			break;
 		case '*':
 			if (!dot) {
@@ -301,7 +309,7 @@ reswitch:	switch (ch = (u_char)*fmt++) {
 			num = (u_int)va_arg(ap, int);
 			p = va_arg(ap, char *);
 			for (q = dtrace_debug_ksprintn(nbuf, num, *p++, NULL, 0); *q;)
-				dtrace_debug__putc(*q--);
+				dtrace_debug__putc(cpu, *q--);
 
 			if (num == 0)
 				break;
@@ -309,19 +317,19 @@ reswitch:	switch (ch = (u_char)*fmt++) {
 			for (tmp = 0; *p;) {
 				n = *p++;
 				if (num & (1 << (n - 1))) {
-					dtrace_debug__putc(tmp ? ',' : '<');
+					dtrace_debug__putc(cpu, tmp ? ',' : '<');
 					for (; (n = *p) > ' '; ++p)
-						dtrace_debug__putc(n);
+						dtrace_debug__putc(cpu, n);
 					tmp = 1;
 				} else
 					for (; *p > ' '; ++p)
 						continue;
 			}
 			if (tmp)
-				dtrace_debug__putc('>');
+				dtrace_debug__putc(cpu, '>');
 			break;
 		case 'c':
-			dtrace_debug__putc(va_arg(ap, int));
+			dtrace_debug__putc(cpu, va_arg(ap, int));
 			break;
 		case 'D':
 			up = va_arg(ap, u_char *);
@@ -329,12 +337,12 @@ reswitch:	switch (ch = (u_char)*fmt++) {
 			if (!width)
 				width = 16;
 			while(width--) {
-				dtrace_debug__putc(hex2ascii(*up >> 4));
-				dtrace_debug__putc(hex2ascii(*up & 0x0f));
+				dtrace_debug__putc(cpu, hex2ascii(*up >> 4));
+				dtrace_debug__putc(cpu, hex2ascii(*up & 0x0f));
 				up++;
 				if (width)
 					for (q=p;*q;q++)
-						dtrace_debug__putc(*q);
+						dtrace_debug__putc(cpu, *q);
 			}
 			break;
 		case 'd':
@@ -406,12 +414,12 @@ reswitch:	switch (ch = (u_char)*fmt++) {
 
 			if (!ladjust && width > 0)
 				while (width--)
-					dtrace_debug__putc(padc);
+					dtrace_debug__putc(cpu, padc);
 			while (n--)
-				dtrace_debug__putc(*p++);
+				dtrace_debug__putc(cpu, *p++);
 			if (ladjust && width > 0)
 				while (width--)
-					dtrace_debug__putc(padc);
+					dtrace_debug__putc(cpu, padc);
 			break;
 		case 't':
 			tflag = 1;
@@ -485,32 +493,32 @@ number:
 			if (!ladjust && padc != '0' && width
 			    && (width -= tmp) > 0)
 				while (width--)
-					dtrace_debug__putc(padc);
+					dtrace_debug__putc(cpu, padc);
 			if (neg)
-				dtrace_debug__putc('-');
+				dtrace_debug__putc(cpu, '-');
 			if (sharpflag && num != 0) {
 				if (base == 8) {
-					dtrace_debug__putc('0');
+					dtrace_debug__putc(cpu, '0');
 				} else if (base == 16) {
-					dtrace_debug__putc('0');
-					dtrace_debug__putc('x');
+					dtrace_debug__putc(cpu, '0');
+					dtrace_debug__putc(cpu, 'x');
 				}
 			}
 			if (!ladjust && width && (width -= tmp) > 0)
 				while (width--)
-					dtrace_debug__putc(padc);
+					dtrace_debug__putc(cpu, padc);
 
 			while (*p)
-				dtrace_debug__putc(*p--);
+				dtrace_debug__putc(cpu, *p--);
 
 			if (ladjust && width && (width -= tmp) > 0)
 				while (width--)
-					dtrace_debug__putc(padc);
+					dtrace_debug__putc(cpu, padc);
 
 			break;
 		default:
 			while (percent < fmt)
-				dtrace_debug__putc(*percent++);
+				dtrace_debug__putc(cpu, *percent++);
 			/*
 			 * Since we ignore an formatting argument it is no 
 			 * longer safe to obey the remaining formatting
@@ -522,23 +530,25 @@ number:
 		}
 	}
 
-	dtrace_debug__putc('\0');
+	dtrace_debug__putc(cpu, '\0');
 }
 
 void
 dtrace_debug_printf(const char *fmt, ...)
 {
 	va_list ap;
+	int cpu;
 
-	dtrace_debug_lock(curcpu);
+	cpu = curcpu;
+	dtrace_debug_lock(cpu);
 
 	va_start(ap, fmt);
 
-	dtrace_debug_vprintf(fmt, ap);
+	dtrace_debug_vprintf(cpu, fmt, ap);
 
 	va_end(ap);
 
-	dtrace_debug_unlock(curcpu);
+	dtrace_debug_unlock(cpu);
 }
 
 #else