Skip to content

Unify global umf_ba_pool_t initialization #229

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

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

igchor
Copy link
Member

@igchor igchor commented Feb 12, 2024

Initializing the global pool in libumf and in each pool separately led to problems with double initialization when multiple pools were used in a single application (and static libumf).

To avoid this problem, use util_init_once inside umf_ba_get_pool instead of having explicit constructor/destructor.

This fixes an issue exposed by: 0c55d8f

Initializing the global pool in libumf and in each pool
separately led to problems with double initialization when
multiple pools were used in a single application (and static libumf).

To avoid this problem, use util_init_once inside umf_ba_get_pool instead
of having explicit constructor/destructor
@igchor igchor requested a review from a team as a code owner February 12, 2024 17:56
@igchor
Copy link
Member Author

igchor commented Feb 12, 2024

This should have been tested by #223 but scalable pool was missing from the test. It's fixed here: #228

Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

LGTM

remove unnecessary &
Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

@igchor There is an issue here. I run ubench with proxy library in LD_PRELOAD:

$ LD_PRELOAD=./lib/libumf_proxy.so ./benchmark/ubench
  1. The constructor of libumf_proxy.so is called.
  2. The constructor runs umfMemoryProviderCreate(umfOsMemoryProviderOps()) that creates the global base allocator no. 1
  3. The constructor runs umfPoolCreate(umfJemallocPoolOps()) that creates the global base allocator no. 2 and allocates je_malloc pool inside it.
  4. The benchmark runs and ends.
  5. atexit(umf_ba_destroy_global); is called and the global base allocator no. 2 is destroyed.
  6. The destructor of libumf_proxy.so is called.
  7. The destructor of libumf_proxy.so runs umfPoolDestroy(pool) and it blows up, because the memory it uses has already been freed in 5)

Copy link
Contributor

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

However I think it should be merged as is, because it fixes the failure of #227 and refined later.

@bratpiorka bratpiorka merged commit 7becea0 into oneapi-src:main Feb 13, 2024
@kswiecicki kswiecicki mentioned this pull request Feb 13, 2024
2 tasks
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