Patchwork [1/2] Add mutex around semaphore counting (LP:#1017388)

login
register
mail settings
Submitter Colin King
Date July 16, 2012, 6:08 p.m.
Message ID <1342462140-16674-2-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/171231/
State Accepted
Headers show

Comments

Colin King - July 16, 2012, 6:08 p.m.
From: Colin Ian King <colin.king@canonical.com>

The ACPICA core can execute control methods in a non
serialized manner, so we should add a mutex lock around
the semaphore counting mechanism to remove any race
conditions.  This fixes bug LP:#1017388

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpica/fwts_acpica.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Alex Hung - July 18, 2012, 2:04 a.m.
On 07/17/2012 02:08 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The ACPICA core can execute control methods in a non
> serialized manner, so we should add a mutex lock around
> the semaphore counting mechanism to remove any race
> conditions.  This fixes bug LP:#1017388
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index b8119cb..cf9a4fe 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -29,6 +29,9 @@
>   #include <errno.h>
>   #include <semaphore.h>
>   #include <signal.h>
> +#include <pthread.h>
> +
> +static pthread_mutex_t mutex_lock_count;
>
>   #include "fwts.h"
>
> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>   {
>   	int i;
>
> +	pthread_mutex_lock(&mutex_lock_count);
> +
>   	for (i=0;i<MAX_SEMAPHORES;i++) {
>   		sem_hash_table[i].sem = NULL;
>   		sem_hash_table[i].count = 0;
>   	}
> +
> +	pthread_mutex_unlock(&mutex_lock_count);
>   }
>
>   /*
> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>   static void hash_sem_inc_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count++;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>   static void hash_sem_dec_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count--;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>   	if (fwts_acpica_init_called)
>   		return FWTS_ERROR;
>
> +	pthread_mutex_init(&mutex_lock_count, NULL);
> +
>   	fwts_acpica_fw = fw;
>
>   	AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>   		return FWTS_ERROR;
>
>   	AcpiTerminate();
> +	pthread_mutex_destroy(&mutex_lock_count);
>
>   	FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>   	FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>

The patch looks good and necessary and worthes an ACK; however, fwts 
still generates the error messages: FAILED [MEDIUM] AMLLocksAcquired: 
Test 61, \_WAK left 1 locks in an acquired state

I think it occurs less frequently. Can someone help test it, too?

PS. I also attach the result (10 times of fwts method").
Ivan Hu - July 18, 2012, 2:11 a.m.
On 07/17/2012 02:08 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The ACPICA core can execute control methods in a non
> serialized manner, so we should add a mutex lock around
> the semaphore counting mechanism to remove any race
> conditions.  This fixes bug LP:#1017388
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index b8119cb..cf9a4fe 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -29,6 +29,9 @@
>   #include <errno.h>
>   #include <semaphore.h>
>   #include <signal.h>
> +#include <pthread.h>
> +
> +static pthread_mutex_t mutex_lock_count;
>
>   #include "fwts.h"
>
> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>   {
>   	int i;
>
> +	pthread_mutex_lock(&mutex_lock_count);
> +
>   	for (i=0;i<MAX_SEMAPHORES;i++) {
>   		sem_hash_table[i].sem = NULL;
>   		sem_hash_table[i].count = 0;
>   	}
> +
> +	pthread_mutex_unlock(&mutex_lock_count);
>   }
>
>   /*
> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>   static void hash_sem_inc_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count++;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>   static void hash_sem_dec_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count--;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>   	if (fwts_acpica_init_called)
>   		return FWTS_ERROR;
>
> +	pthread_mutex_init(&mutex_lock_count, NULL);
> +
>   	fwts_acpica_fw = fw;
>
>   	AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>   		return FWTS_ERROR;
>
>   	AcpiTerminate();
> +	pthread_mutex_destroy(&mutex_lock_count);
>
>   	FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>   	FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>

Acked-by: Ivan Hu<ivan.hu@canonical.com>
Colin King - July 18, 2012, 7:11 a.m.
On 17/07/12 19:04, Alex Hung wrote:
> On 07/17/2012 02:08 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The ACPICA core can execute control methods in a non
>> serialized manner, so we should add a mutex lock around
>> the semaphore counting mechanism to remove any race
>> conditions.  This fixes bug LP:#1017388
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
>> index b8119cb..cf9a4fe 100644
>> --- a/src/acpica/fwts_acpica.c
>> +++ b/src/acpica/fwts_acpica.c
>> @@ -29,6 +29,9 @@
>>   #include <errno.h>
>>   #include <semaphore.h>
>>   #include <signal.h>
>> +#include <pthread.h>
>> +
>> +static pthread_mutex_t mutex_lock_count;
>>
>>   #include "fwts.h"
>>
>> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>>   {
>>       int i;
>>
>> +    pthread_mutex_lock(&mutex_lock_count);
>> +
>>       for (i=0;i<MAX_SEMAPHORES;i++) {
>>           sem_hash_table[i].sem = NULL;
>>           sem_hash_table[i].count = 0;
>>       }
>> +
>> +    pthread_mutex_unlock(&mutex_lock_count);
>>   }
>>
>>   /*
>> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>>   static void hash_sem_inc_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count++;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>>   static void hash_sem_dec_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count--;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>>       if (fwts_acpica_init_called)
>>           return FWTS_ERROR;
>>
>> +    pthread_mutex_init(&mutex_lock_count, NULL);
>> +
>>       fwts_acpica_fw = fw;
>>
>>       AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
>> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>>           return FWTS_ERROR;
>>
>>       AcpiTerminate();
>> +    pthread_mutex_destroy(&mutex_lock_count);
>>
>>       FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>>       FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>>
>
> The patch looks good and necessary and worthes an ACK; however, fwts
> still generates the error messages: FAILED [MEDIUM] AMLLocksAcquired:
> Test 61, \_WAK left 1 locks in an acquired state
>
> I think it occurs less frequently. Can someone help test it, too?

Can you dump the acpitables and send them to me (off the mailing list) 
so I can see why you are seeing the locking issue?

Thanks, Colin
>
> PS. I also attach the result (10 times of fwts method").
>
>
>
Colin King - July 19, 2012, 6:59 a.m.
On 17/07/12 19:04, Alex Hung wrote:
> On 07/17/2012 02:08 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The ACPICA core can execute control methods in a non
>> serialized manner, so we should add a mutex lock around
>> the semaphore counting mechanism to remove any race
>> conditions.  This fixes bug LP:#1017388
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
>> index b8119cb..cf9a4fe 100644
>> --- a/src/acpica/fwts_acpica.c
>> +++ b/src/acpica/fwts_acpica.c
>> @@ -29,6 +29,9 @@
>>   #include <errno.h>
>>   #include <semaphore.h>
>>   #include <signal.h>
>> +#include <pthread.h>
>> +
>> +static pthread_mutex_t mutex_lock_count;
>>
>>   #include "fwts.h"
>>
>> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>>   {
>>       int i;
>>
>> +    pthread_mutex_lock(&mutex_lock_count);
>> +
>>       for (i=0;i<MAX_SEMAPHORES;i++) {
>>           sem_hash_table[i].sem = NULL;
>>           sem_hash_table[i].count = 0;
>>       }
>> +
>> +    pthread_mutex_unlock(&mutex_lock_count);
>>   }
>>
>>   /*
>> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>>   static void hash_sem_inc_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count++;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>>   static void hash_sem_dec_count(sem_t *sem)
>>   {
>>       unsigned int i = hash_sem_handle(sem);
>> +
>>       if (i != HASH_FULL) {
>> +        pthread_mutex_lock(&mutex_lock_count);
>>           sem_hash_table[i].sem = sem;
>>           sem_hash_table[i].count--;
>> +        pthread_mutex_unlock(&mutex_lock_count);
>>       }
>>   }
>>
>> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>>       if (fwts_acpica_init_called)
>>           return FWTS_ERROR;
>>
>> +    pthread_mutex_init(&mutex_lock_count, NULL);
>> +
>>       fwts_acpica_fw = fw;
>>
>>       AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
>> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>>           return FWTS_ERROR;
>>
>>       AcpiTerminate();
>> +    pthread_mutex_destroy(&mutex_lock_count);
>>
>>       FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>>       FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>>
>
> The patch looks good and necessary and worthes an ACK; however, fwts
> still generates the error messages: FAILED [MEDIUM] AMLLocksAcquired:
> Test 61, \_WAK left 1 locks in an acquired state
>
> I think it occurs less frequently. Can someone help test it, too?

After a lot of debugging tonight I found that this occurs in _WAK notify 
events. The notify is creating a thread which tries to execute _NOT 
which does not exit before the _WAK completes, hence we can end up with 
a notify thread still holding a mutex which causes the acquired lock 
issue.  At least I now know why it's failing.  Now I need to figure out 
a suitable fix.

Colin
>
> PS. I also attach the result (10 times of fwts method").
>
>
>
Alex Hung - July 19, 2012, 7:09 a.m.
On 07/17/2012 02:08 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The ACPICA core can execute control methods in a non
> serialized manner, so we should add a mutex lock around
> the semaphore counting mechanism to remove any race
> conditions.  This fixes bug LP:#1017388
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpica/fwts_acpica.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index b8119cb..cf9a4fe 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -29,6 +29,9 @@
>   #include <errno.h>
>   #include <semaphore.h>
>   #include <signal.h>
> +#include <pthread.h>
> +
> +static pthread_mutex_t mutex_lock_count;
>
>   #include "fwts.h"
>
> @@ -109,10 +112,14 @@ void fwts_acpica_sem_count_clear(void)
>   {
>   	int i;
>
> +	pthread_mutex_lock(&mutex_lock_count);
> +
>   	for (i=0;i<MAX_SEMAPHORES;i++) {
>   		sem_hash_table[i].sem = NULL;
>   		sem_hash_table[i].count = 0;
>   	}
> +
> +	pthread_mutex_unlock(&mutex_lock_count);
>   }
>
>   /*
> @@ -171,9 +178,12 @@ static unsigned int hash_sem_handle(sem_t *sem)
>   static void hash_sem_inc_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count++;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -184,9 +194,12 @@ static void hash_sem_inc_count(sem_t *sem)
>   static void hash_sem_dec_count(sem_t *sem)
>   {
>   	unsigned int i = hash_sem_handle(sem);
> +
>   	if (i != HASH_FULL) {
> +		pthread_mutex_lock(&mutex_lock_count);
>   		sem_hash_table[i].sem = sem;
>   		sem_hash_table[i].count--;
> +		pthread_mutex_unlock(&mutex_lock_count);
>   	}
>   }
>
> @@ -788,6 +801,8 @@ int fwts_acpica_init(fwts_framework *fw)
>   	if (fwts_acpica_init_called)
>   		return FWTS_ERROR;
>
> +	pthread_mutex_init(&mutex_lock_count, NULL);
> +
>   	fwts_acpica_fw = fw;
>
>   	AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
> @@ -974,6 +989,7 @@ int fwts_acpica_deinit(void)
>   		return FWTS_ERROR;
>
>   	AcpiTerminate();
> +	pthread_mutex_destroy(&mutex_lock_count);
>
>   	FWTS_ACPICA_FREE(fwts_acpica_XSDT);
>   	FWTS_ACPICA_FREE(fwts_acpica_RSDT);
>
Acked-by: Alex Hung <alex.hung@canonical.com>

Patch

diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
index b8119cb..cf9a4fe 100644
--- a/src/acpica/fwts_acpica.c
+++ b/src/acpica/fwts_acpica.c
@@ -29,6 +29,9 @@ 
 #include <errno.h>
 #include <semaphore.h>
 #include <signal.h>
+#include <pthread.h>
+
+static pthread_mutex_t mutex_lock_count;
 
 #include "fwts.h"
 
@@ -109,10 +112,14 @@  void fwts_acpica_sem_count_clear(void)
 {
 	int i;
 
+	pthread_mutex_lock(&mutex_lock_count);
+
 	for (i=0;i<MAX_SEMAPHORES;i++) {
 		sem_hash_table[i].sem = NULL;
 		sem_hash_table[i].count = 0;
 	}
+
+	pthread_mutex_unlock(&mutex_lock_count);
 }
 
 /*
@@ -171,9 +178,12 @@  static unsigned int hash_sem_handle(sem_t *sem)
 static void hash_sem_inc_count(sem_t *sem)
 {
 	unsigned int i = hash_sem_handle(sem);
+
 	if (i != HASH_FULL) {
+		pthread_mutex_lock(&mutex_lock_count);
 		sem_hash_table[i].sem = sem;
 		sem_hash_table[i].count++;
+		pthread_mutex_unlock(&mutex_lock_count);
 	}
 }
 
@@ -184,9 +194,12 @@  static void hash_sem_inc_count(sem_t *sem)
 static void hash_sem_dec_count(sem_t *sem)
 {
 	unsigned int i = hash_sem_handle(sem);
+
 	if (i != HASH_FULL) {
+		pthread_mutex_lock(&mutex_lock_count);
 		sem_hash_table[i].sem = sem;
 		sem_hash_table[i].count--;
+		pthread_mutex_unlock(&mutex_lock_count);
 	}
 }
 
@@ -788,6 +801,8 @@  int fwts_acpica_init(fwts_framework *fw)
 	if (fwts_acpica_init_called)
 		return FWTS_ERROR;
 
+	pthread_mutex_init(&mutex_lock_count, NULL);
+
 	fwts_acpica_fw = fw;
 
 	AcpiDbgLevel = ACPI_NORMAL_DEFAULT;
@@ -974,6 +989,7 @@  int fwts_acpica_deinit(void)
 		return FWTS_ERROR;
 
 	AcpiTerminate();
+	pthread_mutex_destroy(&mutex_lock_count);
 
 	FWTS_ACPICA_FREE(fwts_acpica_XSDT);
 	FWTS_ACPICA_FREE(fwts_acpica_RSDT);