-
Notifications
You must be signed in to change notification settings - Fork 423
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
UCM: added madvise into VM_UNMAP events group #3836
UCM: added madvise into VM_UNMAP events group #3836
Conversation
src/ucm/mmap/install.c
Outdated
UCM_FIRE_EVENT(events, UCM_EVENT_MMAP|UCM_EVENT_VM_MAPPED, data, | ||
p = mmap(NULL, ucm_get_page_size(), PROT_READ|PROT_WRITE, | ||
MAP_PRIVATE|MAP_ANON, -1, 0)); | ||
if (p != MAP_FAILED) { | ||
UCM_FIRE_EVENT(events, UCM_EVENT_MADVISE, data, | ||
madvise(p, ucm_get_page_size(), MADV_NORMAL)); | ||
madvise(p, ucm_get_page_size(), | ||
#if HAVE_DECL_MADV_REMOVE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we reuse some code with ucm_madvise()?
maybe if none of the MADV_xx events which actually release memory is not defined - don't test it, since we would ne be able to handle it anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch BTW! we should probably add it to gtest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we skip this test based on MADV_ macro then we have to add such preprocessings into all locations where event groups are defined and used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw why not just use MADV_DONTNEED?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MADV_DONTNEED doesn't free pages - didn't expect that it produced event... fixed.
also added gtset for madvise (and bit updated gtest infra for malloc hooks)
Test PASSed. |
Test PASSed. |
ba8bc13
to
e12db21
Compare
forced push to fix typo |
Test FAILed. |
bot:bgate:retest |
Test PASSed. |
@@ -132,6 +132,39 @@ class malloc_hook : public ucs::test { | |||
UCM_BISTRO_EPILOGUE; | |||
return res; | |||
} | |||
|
|||
static int madvise(void *addr, size_t length, int advise) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add also a non-bistro test? for example add madvise(DONTNEED) to void test_thread::test()
- to test also reloc mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to mmap_hooks::test
Test FAILed. |
Mellanox CI: FAILED on 14 of 37 workers (click for details)Note: the logs will be deleted after 13-Jul-2019
|
Test PASSed. |
Test FAILed. |
Test FAILed. |
Mellanox CI: FAILED on 20 of 37 workers (click for details)Note: the logs will be deleted after 13-Jul-2019
|
Test FAILed. |
Mellanox CI: FAILED on 21 of 37 workers (click for details)Note: the logs will be deleted after 13-Jul-2019
|
bot:pipe:retest |
test/gtest/ucm/malloc_hook.cc
Outdated
/* 7. Unmap the buffer */ | ||
/* 7. Unmap the buffer using madvise */ | ||
{ | ||
madvise(buffer, size, MADV_DONTNEED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think it's the right place to add this test
it should be in void test_thread::test()
allocate a new buffer and madvise(DONTNEED) it ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, shifted
Test PASSed. |
Test FAILed. |
Mellanox CI: FAILED on 3 of 37 workers (click for details)Note: the logs will be deleted after 15-Jul-2019
|
Test FAILed. |
Mellanox CI: FAILED on 3 of 37 workers (click for details)Note: the logs will be deleted after 15-Jul-2019
|
Test PASSed. |
bot:mlx:retest |
Test PASSed. |
Mellanox CI: PASSED on 33 workers (click for details)Note: the logs will be deleted after 16-Jul-2019
|
- added gtest for missed madvise event - added test for madvise call
d5d64b0
to
51a14c3
Compare
squashed |
Test PASSed. |
Test FAILed. |
Mellanox CI: FAILED on 3 of 33 workers (click for details)Note: the logs will be deleted after 16-Jul-2019
|
out-of-memory fail |
Test FAILed. |
Mellanox CI: FAILED on 2 of 33 workers (click for details)Note: the logs will be deleted after 16-Jul-2019
|
bot:mlx:retest |
Test PASSed. |
Mellanox CI: PASSED on 33 workers (click for details)Note: the logs will be deleted after 16-Jul-2019
|
fixes #3831