> On Mar 4, 2015, at 5:34 PM, Chen Fan <chen.fan.f...@cn.fujitsu.com> wrote:
>
> Hi Jeff,
>
> Thanks for your explanation, I didn't encounter any issue if leaving it
> there.
>
> I just think if use Volatile qualifier may cause the problem of
> performance. because
> access the value need to copy the address value from main memory to cpu
> cache
> each time and not use the cache.
> and IIRC, in a shared memory multiprocessor system, the cache coherence may
> keep the consistency of shared resource data in each processor local
> caches.
> if the value is updated by one processor in local cache, the copy of
> memory data in
> other processor caches will be marked as invalid. and then other
> processors read the
> value will access from the main memory.
> if I misunderstand, please correct me. :)
>
Correction!
Volatile in C has nothing to do with hardware and caching. It has to do with
the abstract memory model for the compiler as defined by the C standard. The
more aggressive the compiler optimizes the code the more you are exposed to
potential issues. If the compiler knows about all the code in the program that
accesses a variable it can assume that memory location will only be changed by
the compiler, and make optimizations (like storing a pointer in a register).
A common optimization is to avoid memory reads that are not needed. Here is a
simple example in clang, probably the most aggressive optimizing compiler I
know of…. (My comments start with #):
~/work/Compiler>cat volatile.c
int
main (int argc, char **argv)
{
int *i = (int *)0xFFFFFFF0;
while (*i != 0);
return 0;
}
~/work/Compiler>clang volatile.c -S -O3
~/work/Compiler>cat volatile.S
.section __TEXT,__text,regular,pure_instructions
.globl _main
.align 4, 0x90
_main: ## @main
pushq %rbp
movq %rsp, %rbp
movl $4294967280, %eax ## imm = 0xFFFFFFF0
cmpl $0, (%rax)
je LBB0_2
# Notice the compiler only does a single read. The memory model of C implies
this memory will not change.
.align 4, 0x90
LBB0_1: ## %..split_crit_edge
## =>This Inner Loop Header: Depth=1
jmp LBB0_1
# Notice this while loop is just an infinite loop? There is no point reading
the memory since the C spec states it will not change.
LBB0_2: ## %.split1
xorl %eax, %eax
popq %rbp
retq
If I make i a volatile variable it forces the compiler to read it every time:
volatile int *i = (int *)0xFFFFFFF0;
~/work/Compiler>cat volatile.S
.section __TEXT,__text,regular,pure_instructions
.globl _main
.align 4, 0x90
_main: ## @main
pushq %rbp
movq %rsp, %rbp
movl $4294967280, %eax ## imm = 0xFFFFFFF0
.align 4, 0x90
LBB0_1: ## =>This Inner Loop Header: Depth=1
cmpl $0, (%rax)
jne LBB0_1
# Now it reads every iteration of the while loop
xorl %eax, %eax
popq %rbp
retq
So the point of making it volatile is to force the compiler to read the value
every time the C code asks for the value.
Thanks,
Andrew Fish
> Thanks,
> Chen
>
>
>
> On 03/04/2015 03:56 PM, Fan, Jeff wrote:
>> Chen,
>>
>> Volatile is to tell compiler this variable maybe changed by other threads.
>> Compiler must access the value from memory instead of the old value in its
>> registers. This is different with spinlock that is to protect shared
>> memory between multiple threads.
>>
>> For this case, CpuState may be updated by BSP/AP. I prefer to keep volatile
>> type. Do you meet any issue if keepping this type?
>>
>> Besides it, I also suggest to add volatile for the following variable.
>> CpuData->Parameter
>> CpuData->Procedure
>>
>> Thanks!
>> Jeff
>>
>> -----Original Message-----
>> From: Chen Fan [mailto:chen.fan.f...@cn.fujitsu.com]
>> Sent: Tuesday, March 03, 2015 1:06 PM
>> To: edk2-devel@lists.sourceforge.net
>> Cc: Fan, Jeff; Justen, Jordan L
>> Subject: [v2 4/4] UefiCpuPkg/MpSerivce: remove volatile qualifier
>>
>> because manipulating CpuState always with lock protection. so volatile
>> qualifier is redundant.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
>> ---
>> UefiCpuPkg/CpuDxe/CpuMp.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h index
>> cb3460f..e54b571 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
>> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
>> @@ -92,7 +92,7 @@ typedef struct {
>> EFI_PROCESSOR_INFORMATION Info;
>> SPIN_LOCK CpuDataLock;
>> INTN LockSelf;
>> - volatile CPU_STATE State;
>> + CPU_STATE State;
>>
>> EFI_AP_PROCEDURE Procedure;
>> VOID *Parameter;
>> --
>> 1.9.3
>>
>> .
>>
>
>
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming The Go Parallel Website, sponsored
> by Intel and developed in partnership with Slashdot Media, is your hub for all
> things parallel software development, from weekly thought leadership blogs to
> news, videos, case studies, tutorials and more. Take a look and join the
> conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
