From 1c9573a157f0d6d76ded5e651bb3f0b9f3a3c9ec Mon Sep 17 00:00:00 2001 From: Soby Mathew Date: Thu, 19 Feb 2015 16:23:51 +0000 Subject: [PATCH 1/2] Optimize the bakery lock structure for coherent memory This patch optimizes the data structure used with the bakery lock implementation for coherent memory to save memory and minimize memory accesses. These optimizations were already part of the bakery lock implementation for normal memory and this patch now implements it for the coherent memory implementation as well. Also included in the patch is a cleanup to use the do-while loop while waiting for other contenders to finish choosing their tickets. Change-Id: Iedb305473133dc8f12126726d8329b67888b70f1 --- include/lib/bakery_lock.h | 28 ++++++++++++++++++++++--- lib/locks/bakery/bakery_lock_coherent.c | 26 +++++++++++------------ lib/locks/bakery/bakery_lock_normal.c | 22 ++++--------------- 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/include/lib/bakery_lock.h b/include/lib/bakery_lock.h index 9736f850a..eaefecd9e 100644 --- a/include/lib/bakery_lock.h +++ b/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,12 +38,34 @@ #ifndef __ASSEMBLY__ #include +/***************************************************************************** + * 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]; - volatile unsigned number[BAKERY_LOCK_MAX_CPUS]; + /* + * The lock_data is a bit-field of 2 members: + * 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) diff --git a/lib/locks/bakery/bakery_lock_coherent.c b/lib/locks/bakery/bakery_lock_coherent.c index 5d538ce2c..02d31d646 100644 --- a/lib/locks/bakery/bakery_lock_coherent.c +++ b/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,10 +63,6 @@ assert(entry < BAKERY_LOCK_MAX_CPUS); \ } while (0) -/* Convert a ticket to priority */ -#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) { @@ -95,9 +91,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 +103,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->entering[me] = 0; + bakery->lock_data[me] = make_bakery_data(CHOSEN_TICKET, my_ticket); return my_ticket; } @@ -129,6 +124,7 @@ 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()); @@ -150,14 +146,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,7 +164,8 @@ 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])); } } @@ -189,7 +187,7 @@ void bakery_lock_release(bakery_lock_t *bakery) * waiting contenders */ bakery->owner = NO_OWNER; - bakery->number[me] = 0; + bakery->lock_data[me] = 0; dsb(); sev(); } diff --git a/lib/locks/bakery/bakery_lock_normal.c b/lib/locks/bakery/bakery_lock_normal.c index a325fd4fe..4503ae097 100644 --- a/lib/locks/bakery/bakery_lock_normal.c +++ b/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) @@ -138,7 +127,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 +139,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 +163,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 From 548579f56eb95d3d4ba1484a8922a9f6e0a03c73 Mon Sep 17 00:00:00 2001 From: Soby Mathew Date: Fri, 20 Feb 2015 16:04:17 +0000 Subject: [PATCH 2/2] Remove the `owner` field in bakery_lock_t data structure This patch removes the `owner` field in bakery_lock_t structure which is the data structure used in the bakery lock implementation that uses coherent memory. The assertions to protect against recursive lock acquisition were based on the 'owner' field. They are now done based on the bakery lock ticket number. These assertions are also added to the bakery lock implementation that uses normal memory as well. Change-Id: If4850a00dffd3977e218c0f0a8d145808f36b470 --- include/lib/bakery_lock.h | 3 --- lib/locks/bakery/bakery_lock_coherent.c | 16 ++++++---------- lib/locks/bakery/bakery_lock_normal.c | 10 ++++++++++ 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/lib/bakery_lock.h b/include/lib/bakery_lock.h index eaefecd9e..2e1afa21f 100644 --- a/include/lib/bakery_lock.h +++ b/include/lib/bakery_lock.h @@ -58,7 +58,6 @@ #if USE_COHERENT_MEM typedef struct bakery_lock { - int owner; /* * The lock_data is a bit-field of 2 members: * Bit[0] : choosing. This field is set when the CPU is @@ -68,8 +67,6 @@ typedef struct bakery_lock { 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); diff --git a/lib/locks/bakery/bakery_lock_coherent.c b/lib/locks/bakery/bakery_lock_coherent.c index 02d31d646..fd871053a 100644 --- a/lib/locks/bakery/bakery_lock_coherent.c +++ b/lib/locks/bakery/bakery_lock_coherent.c @@ -63,14 +63,13 @@ assert(entry < BAKERY_LOCK_MAX_CPUS); \ } while (0) -/* Initialize Bakery Lock to reset ownership and all ticket values */ +/* Initialize Bakery Lock to reset 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; } @@ -80,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 @@ -130,9 +132,6 @@ void bakery_lock_get(bakery_lock_t *bakery) assert_bakery_entry_valid(me, bakery); - /* Prevent recursive acquisition */ - assert(bakery->owner != me); - /* Get a ticket */ my_ticket = bakery_get_ticket(bakery, me); @@ -168,9 +167,7 @@ void bakery_lock_get(bakery_lock_t *bakery) bakery_ticket_number(bakery->lock_data[they])); } } - /* Lock acquired */ - bakery->owner = me; } @@ -180,13 +177,12 @@ 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; dsb(); sev(); diff --git a/lib/locks/bakery/bakery_lock_normal.c b/lib/locks/bakery/bakery_lock_normal.c index 4503ae097..5439271e9 100644 --- a/lib/locks/bakery/bakery_lock_normal.c +++ b/lib/locks/bakery/bakery_lock_normal.c @@ -87,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. @@ -189,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) @@ -197,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();