From owner-freebsd-acpi@FreeBSD.ORG Fri Mar 1 21:01:15 2013 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id B98085B4; Fri, 1 Mar 2013 21:01:15 +0000 (UTC) (envelope-from jkim@FreeBSD.org) Received: from hammer.pct.niksun.com (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id 4617717C3; Fri, 1 Mar 2013 21:01:15 +0000 (UTC) Message-ID: <513116A1.4070406@FreeBSD.org> Date: Fri, 01 Mar 2013 15:59:13 -0500 From: Jung-uk Kim User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130220 Thunderbird/17.0.3 MIME-Version: 1.0 To: Andriy Gapon Subject: Re: ACPI locking bugs? References: <201302271453.43985.hps@bitfrost.no> <512E1E3D.7090501@FreeBSD.org> In-Reply-To: <512E1E3D.7090501@FreeBSD.org> X-Enigmail-Version: 1.5.1 Content-Type: multipart/mixed; boundary="------------010703080808040408030302" Cc: freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Mar 2013 21:01:15 -0000 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 + * Copyright (c) 2007-2013 Jung-uk Kim * 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--