Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 29 Nov 2012 17:29:15 +0200
From:      Andriy Gapon <avg@FreeBSD.org>
To:        sbruno@FreeBSD.org
Cc:        freebsd-acpi@FreeBSD.org, Sean Bruno <seanwbruno@gmail.com>
Subject:   Re: [rfc] bind curthread to target cpu for _CST change notification
Message-ID:  <50B77F4B.8070701@FreeBSD.org>
In-Reply-To: <1353872249.20189.3.camel@powernoodle>
References:  <50AE3C66.2050207@FreeBSD.org> <1353872249.20189.3.camel@powernoodle>

next in thread | previous in thread | raw e-mail | index | archive | help

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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50B77F4B.8070701>