Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use ABT after calling margo_init #659

Merged
merged 1 commit into from
Aug 9, 2021
Merged

Conversation

adammoody
Copy link
Collaborator

@adammoody adammoody commented Aug 3, 2021

This removes the call to ABT_init and defers that task to Margo. Margo configures ABT in a particular way.

In particular, it increases the default ABT thread stack size from 4KB to 2MB:
https://github.com/mochi-hpc/mochi-margo/blob/ff094a5a26af19041860f9bba1e0e938c0194ae5/src/margo-init.c#L1533-L1534

which resolves some problems as described in:
pmodels/argobots#333

Alternatively, we could call ABT_init before margo_init. However, we likely would need to configure ABT to be compatible with what Margo expects. We need to set some ABT environment variables before calling ABT_init with something like:

    char env_str[64];
    sprintf(env_str, "%d", 2*1024*1024);
    setenv("ABT_THREAD_STACKSIZE", env_str, 0);

    ABT_init(argc, argv);

similar to what is done in margo here:
https://github.com/mochi-hpc/mochi-margo/blob/ff094a5a26af19041860f9bba1e0e938c0194ae5/src/margo-util.c#L29-L36

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Testing (addition of new tests or update to current tests)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the UnifyFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted.

Copy link
Member

@CamStan CamStan left a comment

Choose a reason for hiding this comment

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

Looks good! Assuming @MichaelBrim doesn't have a reason we need to initialize Argobots first.

@adammoody
Copy link
Collaborator Author

Also, we should remember that if we're seeing strange segfaults in the future to try increasing ABT_THREAD_STACK_SIZE. Considering that, it's kind of a bummer that margo calls setenv with overwrite set to 1:

setenv("ABT_THREAD_STACKSIZE", env_str, 1);

https://github.com/mochi-hpc/mochi-margo/blob/ff094a5a26af19041860f9bba1e0e938c0194ae5/src/margo-util.c#L34

That means their setting will overwrite anything the user has set in their environment. Ugh.

I think they do read values from their config file, though:
https://github.com/mochi-hpc/mochi-margo/blob/ff094a5a26af19041860f9bba1e0e938c0194ae5/src/margo-init.c#L1528

I think environment variables are easier to work with if we have to change things. We could ask the margo team about switching the setenv to use overwrite=0.

Copy link
Collaborator

@MichaelBrim MichaelBrim left a comment

Choose a reason for hiding this comment

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

At one time, there was a reason to call ABT_init() separately. I don't remember why now, and can't find a code path that should require that now, so this is OK by me.

@CamStan CamStan mentioned this pull request Aug 4, 2021
@adammoody adammoody merged commit 1e156e2 into LLNL:dev Aug 9, 2021
@adammoody adammoody deleted the abtstacksize branch August 9, 2021 19:10
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