This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Fix a race in task queue reallocation
ClosedPublic

Authored by protze.joachim on May 23 2020, 2:03 PM.

Details

Summary

__kmp_realloc_task_deque implicitly assumes, that the task queue is full (ntasks == size), therefore tail = size in line 319.
I added an assertion to document this assumption.

The first check for a full queue is before the locking and might not hold when the lock is taken. So, we need to check again for this condition when we have the lock.

This fixes the issue for the simple reproducer provided by Raul on openmp-dev, but also my issue with a more complex code.

Diff Detail

Event Timeline

protze.joachim created this revision.May 23 2020, 2:03 PM
openmp/runtime/src/kmp_tasking.cpp
385

Move comment under the if.

3666

Could you please add same check here as well. Though it is harder to reproduce the problem here, the pattern is the same - "get lock + realloc" without re-checking the size of deque under the lock.

Also may worth adding the test just in case.

Addressed Andrey's comment.

I was thinking about a test. The problem I see is that this only manifests under rare circumstances or in complex code.
The best I could think of is adding additional assertions: (head + ntasks) & (size-1) == tail

This revision is now accepted and ready to land.May 25 2020, 12:32 AM
This revision was automatically updated to reflect the committed changes.