Merge pull request #277 from soby-mathew/sm/coh_lock_opt

Optimize the bakery lock implementation

include/lib/bakery_lock.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013-2014, ARM Limited and Contributors. All rights reserved.
+ * Copyright (c) 2013-2015, ARM Limited and Contributors. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
@@ -38,16 +38,35 @@
 #ifndef __ASSEMBLY__
 #include <stdint.h>
 
+/*****************************************************************************
+ * Internal helper macros used by the bakery lock implementation.
+ ****************************************************************************/
+/* Convert a ticket to priority */
+#define PRIORITY(t, pos)	(((t) << 8) | (pos))
+
+#define CHOOSING_TICKET		0x1
+#define CHOSEN_TICKET		0x0
+
+#define bakery_is_choosing(info)	(info & 0x1)
+#define bakery_ticket_number(info)	((info >> 1) & 0x7FFF)
+#define make_bakery_data(choosing, number) \
+		(((choosing & 0x1) | (number << 1)) & 0xFFFF)
+
+/*****************************************************************************
+ * External bakery lock interface.
+ ****************************************************************************/
 #if USE_COHERENT_MEM
 
 typedef struct bakery_lock {
-	int owner;
+	/*
-	volatile char entering[BAKERY_LOCK_MAX_CPUS];
+	 * The lock_data is a bit-field of 2 members:
-	volatile unsigned number[BAKERY_LOCK_MAX_CPUS];
+	 * Bit[0]       : choosing. This field is set when the CPU is
+	 *                choosing its bakery number.
+	 * Bits[1 - 15] : number. This is the bakery number allocated.
+	 */
+	volatile uint16_t lock_data[BAKERY_LOCK_MAX_CPUS];
 } bakery_lock_t;
 
-#define NO_OWNER (-1)
-
 void bakery_lock_init(bakery_lock_t *bakery);
 void bakery_lock_get(bakery_lock_t *bakery);
 void bakery_lock_release(bakery_lock_t *bakery);

lib/locks/bakery/bakery_lock_coherent.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013-2014, ARM Limited and Contributors. All rights reserved.
+ * Copyright (c) 2013-2015, ARM Limited and Contributors. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
@@ -63,18 +63,13 @@
 	assert(entry < BAKERY_LOCK_MAX_CPUS);		\
 } while (0)
 
-/* Convert a ticket to priority */
+/* Initialize Bakery Lock to reset all ticket values */
-#define PRIORITY(t, pos)	(((t) << 8) | (pos))
-
-
-/* Initialize Bakery Lock to reset ownership and all ticket values */
 void bakery_lock_init(bakery_lock_t *bakery)
 {
 	assert(bakery);
 
 	/* All ticket values need to be 0 */
 	memset(bakery, 0, sizeof(*bakery));
-	bakery->owner = NO_OWNER;
 }
 
 
@@ -84,6 +79,9 @@ static unsigned int bakery_get_ticket(bakery_lock_t *bakery, unsigned int me)
 	unsigned int my_ticket, their_ticket;
 	unsigned int they;
 
+	/* Prevent recursive acquisition */
+	assert(!bakery_ticket_number(bakery->lock_data[me]));
+
 	/*
 	 * Flag that we're busy getting our ticket. All CPUs are iterated in the
 	 * order of their ordinal position to decide the maximum ticket value
@@ -95,9 +93,9 @@ static unsigned int bakery_get_ticket(bakery_lock_t *bakery, unsigned int me)
 	 * value, not the ticket value alone.
 	 */
 	my_ticket = 0;
-	bakery->entering[me] = 1;
+	bakery->lock_data[me] = make_bakery_data(CHOOSING_TICKET, my_ticket);
 	for (they = 0; they < BAKERY_LOCK_MAX_CPUS; they++) {
-		their_ticket = bakery->number[they];
+		their_ticket = bakery_ticket_number(bakery->lock_data[they]);
 		if (their_ticket > my_ticket)
 			my_ticket = their_ticket;
 	}
@@ -107,8 +105,7 @@ static unsigned int bakery_get_ticket(bakery_lock_t *bakery, unsigned int me)
 	 * finish calculating our ticket value that we're done
 	 */
 	++my_ticket;
-	bakery->number[me] = my_ticket;
+	bakery->lock_data[me] = make_bakery_data(CHOSEN_TICKET, my_ticket);
-	bakery->entering[me] = 0;
 
 	return my_ticket;
 }
@@ -129,14 +126,12 @@ void bakery_lock_get(bakery_lock_t *bakery)
 {
 	unsigned int they, me;
 	unsigned int my_ticket, my_prio, their_ticket;
+	unsigned int their_bakery_data;
 
 	me = platform_get_core_pos(read_mpidr_el1());
 
 	assert_bakery_entry_valid(me, bakery);
 
-	/* Prevent recursive acquisition */
-	assert(bakery->owner != me);
-
 	/* Get a ticket */
 	my_ticket = bakery_get_ticket(bakery, me);
 
@@ -150,14 +145,15 @@ void bakery_lock_get(bakery_lock_t *bakery)
 			continue;
 
 		/* Wait for the contender to get their ticket */
-		while (bakery->entering[they])
+		do {
-			;
+			their_bakery_data = bakery->lock_data[they];
+		} while (bakery_is_choosing(their_bakery_data));
 
 		/*
 		 * If the other party is a contender, they'll have non-zero
 		 * (valid) ticket value. If they do, compare priorities
 		 */
-		their_ticket = bakery->number[they];
+		their_ticket = bakery_ticket_number(their_bakery_data);
 		if (their_ticket && (PRIORITY(their_ticket, they) < my_prio)) {
 			/*
 			 * They have higher priority (lower value). Wait for
@@ -167,12 +163,11 @@ void bakery_lock_get(bakery_lock_t *bakery)
 			 */
 			do {
 				wfe();
-			} while (their_ticket == bakery->number[they]);
+			} while (their_ticket ==
+				bakery_ticket_number(bakery->lock_data[they]));
 		}
 	}
-
 	/* Lock acquired */
-	bakery->owner = me;
 }
 
 
@@ -182,14 +177,13 @@ void bakery_lock_release(bakery_lock_t *bakery)
 	unsigned int me = platform_get_core_pos(read_mpidr_el1());
 
 	assert_bakery_entry_valid(me, bakery);
-	assert(bakery->owner == me);
+	assert(bakery_ticket_number(bakery->lock_data[me]));
 
 	/*
-	 * Release lock by resetting ownership and ticket. Then signal other
+	 * Release lock by resetting ticket. Then signal other
 	 * waiting contenders
 	 */
-	bakery->owner = NO_OWNER;
+	bakery->lock_data[me] = 0;
-	bakery->number[me] = 0;
 	dsb();
 	sev();
 }

lib/locks/bakery/bakery_lock_normal.c

@@ -56,17 +56,6 @@
  * accesses regardless of status of address translation.
  */
 
-/* Convert a ticket to priority */
-#define PRIORITY(t, pos)	(((t) << 8) | (pos))
-
-#define CHOOSING_TICKET		0x1
-#define CHOOSING_DONE		0x0
-
-#define bakery_is_choosing(info)	(info & 0x1)
-#define bakery_ticket_number(info)	((info >> 1) & 0x7FFF)
-#define make_bakery_data(choosing, number) \
-		(((choosing & 0x1) | (number << 1)) & 0xFFFF)
-
 /* This macro assumes that the bakery_info array is located at the offset specified */
 #define get_my_bakery_info(offset, id)		\
 	(((bakery_info_t *) (((uint8_t *)_cpu_data()) + offset)) + id)
@@ -98,6 +87,13 @@ static unsigned int bakery_get_ticket(int id, unsigned int offset,
 	my_bakery_info = get_my_bakery_info(offset, id);
 	assert(my_bakery_info);
 
+	/*
+	 * Prevent recursive acquisition.
+	 * Since lock data is written to and cleaned by the owning cpu, it
+	 * doesn't require any cache operations prior to reading the lock data.
+	 */
+	assert(!bakery_ticket_number(my_bakery_info->lock_data));
+
 	/*
 	 * Tell other contenders that we are through the bakery doorway i.e.
 	 * going to allocate a ticket for this cpu.
@@ -138,7 +134,7 @@ static unsigned int bakery_get_ticket(int id, unsigned int offset,
 	 * finish calculating our ticket value that we're done
 	 */
 	++my_ticket;
-	my_bakery_info->lock_data = make_bakery_data(CHOOSING_DONE, my_ticket);
+	my_bakery_info->lock_data = make_bakery_data(CHOSEN_TICKET, my_ticket);
 
 	write_cache_op(my_bakery_info, is_cached);
 
@@ -150,7 +146,7 @@ void bakery_lock_get(unsigned int id, unsigned int offset)
 	unsigned int they, me, is_cached;
 	unsigned int my_ticket, my_prio, their_ticket;
 	bakery_info_t *their_bakery_info;
-	uint16_t their_bakery_data;
+	unsigned int their_bakery_data;
 
 	me = platform_get_core_pos(read_mpidr_el1());
 
@@ -174,15 +170,12 @@ void bakery_lock_get(unsigned int id, unsigned int offset)
 		 */
 		their_bakery_info = get_bakery_info_by_index(offset, id, they);
 		assert(their_bakery_info);
-		read_cache_op(their_bakery_info, is_cached);
-
-		their_bakery_data = their_bakery_info->lock_data;
 
 		/* Wait for the contender to get their ticket */
-		while (bakery_is_choosing(their_bakery_data)) {
+		do {
 			read_cache_op(their_bakery_info, is_cached);
 			their_bakery_data = their_bakery_info->lock_data;
-		}
+		} while (bakery_is_choosing(their_bakery_data));
 
 		/*
 		 * If the other party is a contender, they'll have non-zero
@@ -203,6 +196,7 @@ void bakery_lock_get(unsigned int id, unsigned int offset)
 				== bakery_ticket_number(their_bakery_info->lock_data));
 		}
 	}
+	/* Lock acquired */
 }
 
 void bakery_lock_release(unsigned int id, unsigned int offset)
@@ -211,6 +205,8 @@ void bakery_lock_release(unsigned int id, unsigned int offset)
 	unsigned int is_cached = read_sctlr_el3() & SCTLR_C_BIT;
 
 	my_bakery_info = get_my_bakery_info(offset, id);
+	assert(bakery_ticket_number(my_bakery_info->lock_data));
+
 	my_bakery_info->lock_data = 0;
 	write_cache_op(my_bakery_info, is_cached);
 	sev();