Skip to content

Fixes/tls in comm thread#440

Merged
abouteiller merged 2 commits into
ICLDisco:masterfrom
therault:fixes/tls_in_comm_thread
Nov 2, 2022
Merged

Fixes/tls in comm thread#440
abouteiller merged 2 commits into
ICLDisco:masterfrom
therault:fixes/tls_in_comm_thread

Conversation

@therault

@therault therault commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

Fixes an assert that triggers in debug noisier/paranoid mode after #422

Also remove a warning about unused variable in scheduling.c

@therault therault requested a review from a team as a code owner November 2, 2022 16:05
@therault therault added this to the v4.0 milestone Nov 2, 2022
Comment thread parsec/scheduling.c Outdated
}

ret = __parsec_context_wait( context->virtual_processes[0]->execution_streams[0] );
ret = __parsec_context_wait( parsec_my_execution_stream() );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While technically correct, this change hides the fact that this is in parsec_context_wait, a function that should only be called from the main user thread (aka. context->virtual_processes[0]->execution_streams[0]).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted

Comment thread parsec/parsec.c Outdated
gettimeofday(&tv_now, NULL);

PARSEC_TLS_SET_SPECIFIC(parsec_tls_execution_stream, es);
parsec_set_my_execution_stream(es);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only reason we need the parsec_set_my_execution_stream accessor is because parsec_tls_execution_stream is defined as static, and therefore not visible outside the parsec.c file. I think that where the symbols are visible, the uses of the TLS macros should be encouraged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted

…p_mpi_on directly from the main thread. In this case, we should not change the TLS of the calling thread to point to the comm engine pseudo-execution stream
@therault therault force-pushed the fixes/tls_in_comm_thread branch from e811659 to 8fbfb62 Compare November 2, 2022 18:02
@abouteiller abouteiller merged commit 70ba4e9 into ICLDisco:master Nov 2, 2022
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.

4 participants