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

common: fix lockdep vs recursive mutexes #9940

Merged
merged 3 commits into from Mar 23, 2017

Conversation

Projects
None yet
3 participants
@aclamk
Copy link
Contributor

aclamk commented Jun 27, 2016

The problem is that:
Mutex lock("RGWKeystoneTokenCache", true /* recursive */);
Mutex::Locker l(lock);
lock.Lock();
fails...
Signed-off-by: Adam Kupczyk akupczyk@mirantis.com

@liewegas liewegas changed the title When conf "lockdep = true", recursive mutexes always caused check asserts common: fix lockdep vs recursive mutexes Jun 27, 2016

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Jun 27, 2016

Do you mind creating a unit test for this? We don't actually have any lockdep unit tests, but just a few basic ones would be really nice. (e.g., demonstrate that a nonrecursive AA or ABBA will assert, and that a recursive AA is okay. gtest has the assert macros that assert that the test aborts.)

@aclamk

This comment has been minimized.

Copy link
Contributor Author

aclamk commented Jun 28, 2016

Ok, I will gladly write some unit tests.

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Nov 3, 2016

LGTM, but needs a rebase now that we're cmake!

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jan 18, 2017

@aclamk ping?

@aclamk aclamk force-pushed the aclamk:common-recursive-mutex-fix branch from 2074376 to a28a063 Jan 20, 2017

@aclamk

This comment has been minimized.

Copy link
Contributor Author

aclamk commented Jan 20, 2017

@tchaikov rebased.

*/

#include <common/Mutex.h>
#include "gtest/gtest.h"

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 20, 2017

Contributor

might want to update the CMakeLists.txt to include this test.

This comment has been minimized.

Copy link
@aclamk

aclamk Jan 20, 2017

Author Contributor

@tchaikov: The unittest_mutex_debug is already a part of src/test/common/CMakeLists.txt, lines 191-194. Can you confirm?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jan 20, 2017

Contributor

@aclamk it's "test_mutex_debug.cc" not "test_mutex.cc".

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jan 20, 2017

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

@aclamk aclamk force-pushed the aclamk:common-recursive-mutex-fix branch from a28a063 to 1e8a878 Jan 20, 2017

@aclamk aclamk force-pushed the aclamk:common-recursive-mutex-fix branch from ec0ed2a to b8ef1bf Jan 30, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 8, 2017

@aclamk f0fe3f6 is already merged in #12947. could you remove it from this PR?

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Mar 1, 2017

retest this please.

@tchaikov tchaikov added the needs-qa label Mar 7, 2017

@tchaikov tchaikov self-assigned this Mar 7, 2017

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Mar 10, 2017

can you drop the ceph-disk patch from this PR please?

aclamk added some commits Jun 27, 2016

common: When conf "lockdep = true", recursive mutexes always caused c…
…heck asserts.

Signed-off-by: Adam Kupczyk <akupczyk@mirantis.com>
common: Added tests for mutex
Signed-off-by: Adam Kupczyk <akupczyk@mirantis.com>
Added test_mutex to unittests.
Signed-off-by: Adam Kupczyk <akupczyk@mirantis.com>

@aclamk aclamk force-pushed the aclamk:common-recursive-mutex-fix branch from b8ef1bf to b08d688 Mar 13, 2017

@aclamk

This comment has been minimized.

Copy link
Contributor Author

aclamk commented Mar 13, 2017

@liewegas Dropped ceph-disk patch. I have no idea how it even could get there....

@tchaikov tchaikov merged commit 136c28f into ceph:master Mar 23, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.