Skip to content

Don't pass the execution stream around to recursive calls.#413

Merged
bosilca merged 1 commit intoICLDisco:masterfrom
bosilca:fix/profiling_with_recursive_calls
Aug 11, 2022
Merged

Don't pass the execution stream around to recursive calls.#413
bosilca merged 1 commit intoICLDisco:masterfrom
bosilca:fix/profiling_with_recursive_calls

Conversation

@bosilca
Copy link
Copy Markdown
Contributor

@bosilca bosilca commented Aug 10, 2022

Thanks @omor1 for the bug report.

Fixes #412.

Signed-off-by: George Bosilca bosilca@icl.utk.edu

Signed-off-by: George Bosilca <bosilca@icl.utk.edu>
Copy link
Copy Markdown
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I am not sure I grasp what recursive tasks are ^^

@bosilca bosilca merged commit 7897167 into ICLDisco:master Aug 11, 2022
@omor1
Copy link
Copy Markdown
Contributor

omor1 commented Aug 12, 2022

Whoops, you call parsec_context_add_taskpool( es->virtual_process->parsec_context, tp) but don't have es. Can probably just change it to passing in context.

Edit: that's a nicer API, but much more annoying from the user perspective, since every caller needs to get context and the easiest way to do that is from es->virtual_process->parsec_context (or alternatively from or this_task->taskpool->context)

Edit 2: wait, I'm silly, it's already being passed this_task, we can get the context from there.

@@ -72,7 +70,7 @@ parsec_recursivecall( parsec_execution_stream_t    *es,
     parsec_taskpool_set_complete_callback( tp, &parsec_recursivecall_callback,
                                            (void *)cbdata );
 
-    parsec_context_add_taskpool( es->virtual_process->parsec_context, tp);
+    parsec_context_add_taskpool( task->taskpool->context, tp );
 
     return -1;
 }

abouteiller added a commit to abouteiller/parsec that referenced this pull request Aug 12, 2022
Signed-off-by: Aurelien Bouteiller <bouteill@icl.utk.edu>
omor1 added a commit to uiuc-hpc/parsec that referenced this pull request Aug 17, 2022
PR ICLDisco#413 fixed issues around recursive tasks by retrieving the current
execution stream via `parsec_my_execution_stream`. However, under some
circumstances, it appears that the `on_complete` callback can be called
from the communication thread. The comm thread never sets the TLS
variable needed for `parsec_my_execution_stream`, so you end up with a
NULL execution stream and everything breaks.

Ensure that `parsec_tls_execution_stream` is set by the communication
thread as well. This introduces a new internal API to do so,
`parsec_set_my_execution_stream`, since `parsec_tls_execution_stream` is
a static variable in parsec.c.

Signed-off-by: Omri Mor <omrimor2@illinois.edu>
omor1 added a commit to uiuc-hpc/parsec that referenced this pull request Aug 17, 2022
PR ICLDisco#413 fixed issues around recursive tasks by retrieving the current
execution stream via `parsec_my_execution_stream`. However, under some
circumstances, it appears that the `on_complete` callback can be called
from the communication thread. The comm thread never sets the TLS
variable needed for `parsec_my_execution_stream`, so you end up with a
NULL execution stream and everything breaks.

Ensure that `parsec_tls_execution_stream` is set by the communication
thread as well. This introduces a new internal API to do so,
`parsec_set_my_execution_stream`, since `parsec_tls_execution_stream` is
a static variable in parsec.c.

Signed-off-by: Omri Mor <omrimor2@illinois.edu>
@bosilca bosilca deleted the fix/profiling_with_recursive_calls branch May 19, 2023 21:16
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.

Recursive tasks can generate profiling events on wrong streams (profile corruption)

4 participants