Skip to content

Conversation

@igchor
Copy link
Contributor

@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
Contributor 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