Date: Fri, 01 Mar 2013 15:59:13 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-acpi@freebsd.org Subject: Re: ACPI locking bugs? Message-ID: <513116A1.4070406@FreeBSD.org> In-Reply-To: <512E1E3D.7090501@FreeBSD.org> References: <201302271453.43985.hps@bitfrost.no> <512E1E3D.7090501@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------010703080808040408030302
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 2013-02-27 09:54:53 -0500, Andriy Gapon wrote:
> on 27/02/2013 15:53 Hans Petter Selasky said the following:
>> Hi,
>>
>> What is the reason for using cv_wait_sig() and cv_timedwait_sig()
>> instead of cv_wait() and cv_timedwait() inside of
>> AcpiOsWaitSemaphore(). What signals are supposed to be catched
>> here?
>>
>> switch (Timeout) { case ACPI_DO_NOT_WAIT: if (!ACPISEM_AVAIL(as,
>> Units)) status = AE_TIME; break; case ACPI_WAIT_FOREVER: while
>> (!ACPISEM_AVAIL(as, Units)) { as->as_waiters++; error =
>> cv_wait_sig(&as->as_cv, &as->as_lock); as->as_waiters--; if
>> (error == EINTR || as->as_reset) { status = AE_ERROR; break; } }
>> break; default: tmo = timeout2hz(Timeout); while
>> (!ACPISEM_AVAIL(as, Units)) { prevtick = ticks;
>> as->as_waiters++; error = cv_timedwait_sig(&as->as_cv,
>> &as->as_lock, tmo); as->as_waiters--; if (error == EINTR ||
>> as->as_reset) { status = AE_ERROR; break; } if (ACPISEM_AVAIL(as,
>> Units)) break; slptick = ticks - prevtick; if (slptick >= tmo ||
>> slptick < 0) { status = AE_TIME; break; } tmo -= slptick; } }
When something is miserably waiting forever to grab a mutex/semaphore,
I wanted to be able to kill the misbehaving application, e.g., 'sysctl
- -a' from /etc/rc.d/initrandom. Often times, it is caused by buggy DSDTs.
>> I've observed an issue twice on my MacBookPro that some ACPI
>> locking fails during shutdown on 9-stable and then the power-off
>> won't complete. I've been unable to capture the full dmesg,
>> because syslogd is killed at the moment this happens. There are
>> two ACPI error printouts about failed locking.
I suspect there's a bug in DSDT.
>> I see that in the case of ACPI_WAIT_FOREVER, it appears to be
>> incorrect to catch signals, because sometimes the return argument
>> is not checked for lock operations:
>>
>> ./components/utilities/utosi.c: (void) AcpiOsAcquireMutex
>> (AcpiGbl_OsiMutex, ACPI_WAIT_FOREVER);
>> ./components/utilities/utosi.c: (void) AcpiOsAcquireMutex
>> (AcpiGbl_OsiMutex, ACPI_WAIT_FOREVER);
>> ./components/utilities/utosi.c: (void) AcpiOsAcquireMutex
>> (AcpiGbl_OsiMutex, ACPI_WAIT_FOREVER);
It should be fixed in ACPICA. FYI, here is a patch to address the
problem:
https://github.com/otcshare/acpica/pull/5
IMHO, using ACPI_WAIT_FOREVER is really foot-shooting. In fact, I
believe it should be removed from ACPI specification altogether.
>> Any comments?
>
> kib drew my attention to this issue some time ago and I also pinged
> jkim about it. I completely agree with you that the signal handling
> should be removed. Are you willing to make the change or would you
> prefer me doing it?
Although I don't 100% agree with you, I decided to be more practical.
Please try the attached patch. It is also available from here:
http://people.freebsd.org/~jkim/OsdSynch.diff
I don't think it will fix anything, though.
Jung-uk Kim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iQEcBAEBAgAGBQJRMRagAAoJECXpabHZMqHO+S8H/RnYNxLlLKM2u3s/VdRKXL1I
+WLgfdUpQoWU8RZvb9Kf7QzpiOsmEkEnUIiwYRkEos7YNUSObt8ivWsuxQx/Sxat
nzRl160HOHf9azgz9hlOfmWl1PLK12gMh/wX6E4WvYJLNQed5OXXlkqq9X4DSJ/Q
CVzxcLcKZYkoMFg1wvQcB1nSP4uGHkdtGQc0qB9WWNt4Gmb5T3wfLiy6T3j2YN3x
gchMhvJVTbbGTb5K1eZyahdacXpW3Ost2z/q/mB1eAjUwnsjF0Q95MzGInvIq3n6
wHizY8RHNDWfAyQMPAyqYFIPdbFjDHX6NFHQIIQw3ewn8fKylMi2npls7a9y51k=
=y21z
-----END PGP SIGNATURE-----
--------------010703080808040408030302
Content-Type: text/x-patch;
name="OsdSynch.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="OsdSynch.diff"
Index: sys/dev/acpica/Osd/OsdSynch.c
===================================================================
--- sys/dev/acpica/Osd/OsdSynch.c (revision 247571)
+++ sys/dev/acpica/Osd/OsdSynch.c (working copy)
@@ -1,7 +1,7 @@
/*-
* Copyright (c) 2000 Michael Smith
* Copyright (c) 2000 BSDi
- * Copyright (c) 2007-2009 Jung-uk Kim <jkim@FreeBSD.org>
+ * Copyright (c) 2007-2013 Jung-uk Kim <jkim@FreeBSD.org>
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -45,6 +45,19 @@ __FBSDID("$FreeBSD$");
#define _COMPONENT ACPI_OS_SERVICES
ACPI_MODULE_NAME("SYNCH")
+/*
+ * Allow the user to tune the maximum seconds to destroy mutex and semaphore.
+ */
+static int acpi_wait_destroy = 10;
+TUNABLE_INT("debug.acpi.sync_wait_destroy", &acpi_wait_destroy);
+
+/*
+ * Allow the user to tune the maximum seconds for ACPI_WAIT_FOREVER.
+ * Note it must be larger than 65 seconds.
+ */
+static int acpi_wait_forever = 120;
+TUNABLE_INT("debug.acpi.sync_wait_forever", &acpi_wait_forever);
+
static MALLOC_DEFINE(M_ACPISEM, "acpisem", "ACPI semaphore");
/*
@@ -54,7 +67,15 @@ static int
timeout2hz(UINT16 Timeout)
{
struct timeval tv;
+ int tmo;
+ if (Timeout == ACPI_WAIT_FOREVER) {
+ tmo = acpi_wait_forever * hz;
+ KASSERT(tmo > 0 && acpi_wait_forever > UINT16_MAX / 1000,
+ ("invalid timeout %d", tmo));
+ return (tmo);
+ }
+
tv.tv_sec = (time_t)(Timeout / 1000);
tv.tv_usec = (suseconds_t)(Timeout % 1000) * 1000;
@@ -106,6 +127,7 @@ ACPI_STATUS
AcpiOsDeleteSemaphore(ACPI_SEMAPHORE Handle)
{
struct acpi_sema *as = (struct acpi_sema *)Handle;
+ int retry;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
@@ -122,20 +144,23 @@ AcpiOsDeleteSemaphore(ACPI_SEMAPHORE Handle)
as->as_name, as->as_units, as->as_waiters));
as->as_reset = 1;
cv_broadcast(&as->as_cv);
- while (as->as_waiters > 0) {
- if (mtx_sleep(&as->as_reset, &as->as_lock,
- PCATCH, "acsrst", hz) == EINTR) {
- ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
- "failed to reset %s, waiters %d\n",
- as->as_name, as->as_waiters));
- mtx_unlock(&as->as_lock);
- return_ACPI_STATUS (AE_ERROR);
- }
+ retry = acpi_wait_destroy;
+ while (as->as_waiters > 0 && retry > 0) {
+ mtx_sleep(&as->as_reset, &as->as_lock, PWAIT,
+ "acsrst", hz);
ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
"wait %s, units %u, waiters %d\n",
as->as_name, as->as_units, as->as_waiters));
+ retry--;
}
}
+ if (as->as_waiters > 0) {
+ ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
+ "failed to reset %s, waiters %d\n",
+ as->as_name, as->as_waiters));
+ mtx_unlock(&as->as_lock);
+ return_ACPI_STATUS (AE_ERROR);
+ }
mtx_unlock(&as->as_lock);
@@ -152,7 +177,7 @@ ACPI_STATUS
AcpiOsWaitSemaphore(ACPI_SEMAPHORE Handle, UINT32 Units, UINT16 Timeout)
{
struct acpi_sema *as = (struct acpi_sema *)Handle;
- int error, prevtick, slptick, tmo;
+ int error, prev, tmo;
ACPI_STATUS status = AE_OK;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
@@ -167,50 +192,38 @@ AcpiOsWaitSemaphore(ACPI_SEMAPHORE Handle, UINT32
Units, as->as_name, as->as_units, as->as_waiters, Timeout));
if (as->as_maxunits != ACPI_NO_UNIT_LIMIT && as->as_maxunits < Units) {
- mtx_unlock(&as->as_lock);
- return_ACPI_STATUS (AE_LIMIT);
+ status = AE_LIMIT;
+ goto out;
}
-
- switch (Timeout) {
- case ACPI_DO_NOT_WAIT:
- if (!ACPISEM_AVAIL(as, Units))
+ if (ACPISEM_AVAIL(as, Units)) {
+ as->as_units -= Units;
+ goto out;
+ }
+ if (Timeout == ACPI_DO_NOT_WAIT) {
+ status = AE_TIME;
+ goto out;
+ }
+ tmo = timeout2hz(Timeout);
+ for (;;) {
+ prev = ticks;
+ as->as_waiters++;
+ error = cv_timedwait(&as->as_cv, &as->as_lock, tmo);
+ as->as_waiters--;
+ if (ACPISEM_AVAIL(as, Units)) {
+ as->as_units -= Units;
+ break;
+ }
+ if (as->as_reset || error == EWOULDBLOCK) {
status = AE_TIME;
- break;
- case ACPI_WAIT_FOREVER:
- while (!ACPISEM_AVAIL(as, Units)) {
- as->as_waiters++;
- error = cv_wait_sig(&as->as_cv, &as->as_lock);
- as->as_waiters--;
- if (error == EINTR || as->as_reset) {
- status = AE_ERROR;
- break;
- }
+ break;
}
- break;
- default:
- tmo = timeout2hz(Timeout);
- while (!ACPISEM_AVAIL(as, Units)) {
- prevtick = ticks;
- as->as_waiters++;
- error = cv_timedwait_sig(&as->as_cv, &as->as_lock, tmo);
- as->as_waiters--;
- if (error == EINTR || as->as_reset) {
- status = AE_ERROR;
- break;
- }
- if (ACPISEM_AVAIL(as, Units))
- break;
- slptick = ticks - prevtick;
- if (slptick >= tmo || slptick < 0) {
- status = AE_TIME;
- break;
- }
- tmo -= slptick;
+ tmo -= ticks - prev;
+ if (tmo <= 0) {
+ status = AE_TIME;
+ break;
}
}
- if (ACPI_SUCCESS(status))
- as->as_units -= Units;
-
+out:
mtx_unlock(&as->as_lock);
return_ACPI_STATUS (status);
@@ -296,6 +309,7 @@ void
AcpiOsDeleteMutex(ACPI_MUTEX Handle)
{
struct acpi_mutex *am = (struct acpi_mutex *)Handle;
+ int retry;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
@@ -313,15 +327,10 @@ AcpiOsDeleteMutex(ACPI_MUTEX Handle)
"reset %s, owner %p\n", am->am_name, am->am_owner));
am->am_reset = 1;
wakeup(am);
- while (am->am_waiters > 0) {
- if (mtx_sleep(&am->am_reset, &am->am_lock,
- PCATCH, "acmrst", hz) == EINTR) {
- ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
- "failed to reset %s, waiters %d\n",
- am->am_name, am->am_waiters));
- mtx_unlock(&am->am_lock);
- return_VOID;
- }
+ retry = acpi_wait_destroy;
+ while (am->am_waiters > 0 && retry > 0) {
+ mtx_sleep(&am->am_reset, &am->am_lock, PWAIT,
+ "acmrst", hz);
if (ACPIMTX_AVAIL(am))
ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
"wait %s, waiters %d\n",
@@ -330,8 +339,16 @@ AcpiOsDeleteMutex(ACPI_MUTEX Handle)
ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
"wait %s, owner %p, waiters %d\n",
am->am_name, am->am_owner, am->am_waiters));
+ retry--;
}
}
+ if (am->am_waiters > 0) {
+ ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
+ "failed to reset %s, waiters %d\n",
+ am->am_name, am->am_waiters));
+ mtx_unlock(&am->am_lock);
+ return_VOID;
+ }
mtx_unlock(&am->am_lock);
@@ -343,7 +360,7 @@ ACPI_STATUS
AcpiOsAcquireMutex(ACPI_MUTEX Handle, UINT16 Timeout)
{
struct acpi_mutex *am = (struct acpi_mutex *)Handle;
- int error, prevtick, slptick, tmo;
+ int error, prev, tmo;
ACPI_STATUS status = AE_OK;
ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);
@@ -360,51 +377,37 @@ AcpiOsAcquireMutex(ACPI_MUTEX Handle, UINT16 Timeo
ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
"acquire nested %s, depth %d\n",
am->am_name, am->am_nested));
- mtx_unlock(&am->am_lock);
- return_ACPI_STATUS (AE_OK);
+ goto out;
}
-
- switch (Timeout) {
- case ACPI_DO_NOT_WAIT:
- if (!ACPIMTX_AVAIL(am))
+ if (ACPIMTX_AVAIL(am)) {
+ am->am_owner = curthread;
+ goto out;
+ }
+ if (Timeout == ACPI_DO_NOT_WAIT) {
+ status = AE_TIME;
+ goto out;
+ }
+ tmo = timeout2hz(Timeout);
+ for (;;) {
+ prev = ticks;
+ am->am_waiters++;
+ error = mtx_sleep(am, &am->am_lock, 0, "acmtx", tmo);
+ am->am_waiters--;
+ if (ACPIMTX_AVAIL(am)) {
+ am->am_owner = curthread;
+ break;
+ }
+ if (am->am_reset || error == EWOULDBLOCK) {
status = AE_TIME;
- break;
- case ACPI_WAIT_FOREVER:
- while (!ACPIMTX_AVAIL(am)) {
- am->am_waiters++;
- error = mtx_sleep(am, &am->am_lock, PCATCH, "acmtx", 0);
- am->am_waiters--;
- if (error == EINTR || am->am_reset) {
- status = AE_ERROR;
- break;
- }
+ break;
}
- break;
- default:
- tmo = timeout2hz(Timeout);
- while (!ACPIMTX_AVAIL(am)) {
- prevtick = ticks;
- am->am_waiters++;
- error = mtx_sleep(am, &am->am_lock, PCATCH,
- "acmtx", tmo);
- am->am_waiters--;
- if (error == EINTR || am->am_reset) {
- status = AE_ERROR;
- break;
- }
- if (ACPIMTX_AVAIL(am))
- break;
- slptick = ticks - prevtick;
- if (slptick >= tmo || slptick < 0) {
- status = AE_TIME;
- break;
- }
- tmo -= slptick;
+ tmo -= ticks - prev;
+ if (tmo <= 0) {
+ status = AE_TIME;
+ break;
}
}
- if (ACPI_SUCCESS(status))
- am->am_owner = curthread;
-
+out:
mtx_unlock(&am->am_lock);
return_ACPI_STATUS (status);
--------------010703080808040408030302--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?513116A1.4070406>