Project

General

Profile

Bug #1624

missing fences in CkLoop implementation

Added by Jim Phillips about 1 year ago. Updated 7 months ago.

Status:
Merged
Priority:
High
Category:
Machine Layers
Target version:
Start date:
07/03/2017
Due date:
% Done:

0%


Description

Reported by Sameer Kumar, CkLoop reportFinished() needs a write fence to guarantee that results are visible to the thread that made a sync call to CkLoop (i.e., is waiting for results). Also missing read fence in isFree(). waitLoopDone seems OK because of the locks.

History

#1 Updated by Phil Miller about 1 year ago

  • Target version changed from 6.8.0 to 6.9.0

The group discussed this in core at some point. The code is apparently fine on x86 with its strict memory ordering semantics. Post 6.8, we'll switch to using native C++11 atomics to portably address these issues in full.

#2 Updated by Jim Phillips about 1 year ago

Are CmiMemoryReadFence and CmiMemoryWriteFence somehow not portable and sufficient? Otherwise 6.8.0 CkLoop is broken on both Power and ARM.

#3 Updated by Eric Bohm about 1 year ago

  • Assignee set to Seonmyeong Bak

#4 Updated by Seonmyeong Bak 8 months ago

I think for PPC there are CmiMemoryWrite / CmiMemoryReadFences are inserted correctly.
for x86, we're fine.
So, not sure what the point of this issue is.

#5 Updated by Seonmyeong Bak 8 months ago

  • Status changed from New to In Progress

#6 Updated by Jim Phillips 8 months ago

The point is that Sameer sent a patch to fix this issue on POWER and I assume he knew what he was doing.

#7 Updated by Seonmyeong Bak 8 months ago

  • Status changed from In Progress to Implemented

#9 Updated by Seonmyeong Bak 8 months ago

For memory fences, we use CmiMemoryReadFence() which calls __sync_synchronize()
Maybe less costly fences for 'acquire' in relaxed consistency model are enough for CmiMemoryReadFence() to prevent reordering of loads/stores within the same thread.

__atomic_thread_fence in gcc builtins with specific memory ordering can replace __sync_synchronize.

#10 Updated by Phil Miller 8 months ago

This is all C++ code, and we depend on C++11 now. We can use std::atomic fully.

#11 Updated by Seonmyeong Bak 8 months ago

OK. I'll replace atomics with c++11 atomic codes.

#12 Updated by Phil Miller 7 months ago

  • Status changed from Implemented to Merged

Also available in: Atom PDF