Skip to content

Reshape test: variable used for polling should be volatile#344

Merged
bosilca merged 1 commit intoICLDisco:masterfrom
devreal:fix_reshape_test
Mar 12, 2022
Merged

Reshape test: variable used for polling should be volatile#344
bosilca merged 1 commit intoICLDisco:masterfrom
devreal:fix_reshape_test

Conversation

@devreal
Copy link
Copy Markdown
Contributor

@devreal devreal commented Mar 12, 2022

Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu

Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>
@devreal devreal requested a review from bosilca as a code owner March 12, 2022 01:51
@bosilca bosilca merged commit 9a94e8a into ICLDisco:master Mar 12, 2022
@abouteiller
Copy link
Copy Markdown
Contributor

Don't think that fix is entirely correct https://www.kernel.org/doc/Documentation/process/volatile-considered-harmful.rst

Should have the memory yield instruction in the loop instead.

@ICLDisco/staff

@devreal
Copy link
Copy Markdown
Contributor Author

devreal commented Mar 25, 2022

I agree that volatile is not the perfect choice here, I am not worried about performance in this case though. And we do not have something like cpu_relax() in PaRSEC, nor can we use _Atomic as a replacement for volatile. PaRSEC uses volatile in many places where _Atomic would be a better choice (lifo, list, taskpool, arena, barrier, rwlock, even in the C code generated from JDF). If we want to go down the route of adding cpu_relax() I'm all for it, but it likely requires architecture-specific implementations. We can peek at the kernel code for that, of course. Or just bite the bullet and transition to requiring C11 atomics and allow for _Atomic to be used directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants