From owner-freebsd-acpi@FreeBSD.ORG  Thu Nov 29 15:29:17 2012
Return-Path: <owner-freebsd-acpi@FreeBSD.ORG>
Delivered-To: freebsd-acpi@FreeBSD.org
Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52])
 by hub.freebsd.org (Postfix) with ESMTP id D868A43D;
 Thu, 29 Nov 2012 15:29:17 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140])
 by mx1.freebsd.org (Postfix) with ESMTP id EBD0A8FC12;
 Thu, 29 Nov 2012 15:29:16 +0000 (UTC)
Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua
 [212.40.38.101])
 by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id RAA12122;
 Thu, 29 Nov 2012 17:29:15 +0200 (EET) (envelope-from avg@FreeBSD.org)
Message-ID: <50B77F4B.8070701@FreeBSD.org>
Date: Thu, 29 Nov 2012 17:29:15 +0200
From: Andriy Gapon <avg@FreeBSD.org>
User-Agent: Mozilla/5.0 (X11; FreeBSD amd64;
 rv:17.0) Gecko/17.0 Thunderbird/17.0
MIME-Version: 1.0
To: sbruno@FreeBSD.org
Subject: Re: [rfc] bind curthread to target cpu for _CST change notification
References: <50AE3C66.2050207@FreeBSD.org>
 <1353872249.20189.3.camel@powernoodle>
In-Reply-To: <1353872249.20189.3.camel@powernoodle>
X-Enigmail-Version: 1.4.6
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Cc: freebsd-acpi@FreeBSD.org, Sean Bruno <seanwbruno@gmail.com>
X-BeenThere: freebsd-acpi@freebsd.org
X-Mailman-Version: 2.1.14
Precedence: list
List-Id: ACPI and power management development <freebsd-acpi.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/freebsd-acpi>,
 <mailto:freebsd-acpi-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-acpi>
List-Post: <mailto:freebsd-acpi@freebsd.org>
List-Help: <mailto:freebsd-acpi-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-acpi>,
 <mailto:freebsd-acpi-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 29 Nov 2012 15:29:17 -0000


Turned out not be so rosy...

on 25/11/2012 21:37 Sean Bruno said the following:
> 
> 
> On Thu, 2012-11-22 at 16:53 +0200, Andriy Gapon wrote:
>> I would like to propose the following patch which should kill two birds with one
>> stone:
>>
>> - avoid race in acpi_cpu_cx_cst if more than one notifications occur in a rapid
>> succession for the same CPU and end up being concurrently handled by ACPI taskqeue
>> threads

critical_enter was a very a bad idea and can't be used here because
acpi_cpu_cx_cst acquires blockable locks and does waiting memory allocations.
Unfortunately, it was not immediately obvious to me.

>> - avoid race acpi_cpu_cx_cst and acpi_cpu_idle where the latter may access
>> resources being updated by the former

sched_bind wouldn't guarantee that this would work if critical_enter is not used,
because the current thread may block and the idle thread may get to run.


>> What do you think?
>>
>> Index: sys/dev/acpica/acpi_cpu.c
>> ===================================================================
>> --- sys/dev/acpica/acpi_cpu.c	(revision 242854)
>> +++ sys/dev/acpica/acpi_cpu.c	(working copy)
>> @@ -1047,7 +1047,16 @@
>>  	return;
>>
>>      /* Update the list of Cx states. */
>> +    thread_lock(curthread);
>> +    sched_bind(curthread, sc->cpu_pcpu->pc_cpuid);
>> +    thread_unlock(curthread);
>> +    critical_enter();
>>      acpi_cpu_cx_cst(sc);
>> +    critical_exit();
>> +    thread_lock(curthread);
>> +    sched_unbind(curthread);
>> +    thread_unlock(curthread);
>> +
>>      acpi_cpu_cx_list(sc);
>>
>>      ACPI_SERIAL_BEGIN(cpu);
>>
> 
> Ack.  This looks appropriate to me.

I am working on an alternative approach to these two issues.
Thank you.

-- 
Andriy Gapon