[kudu-CR] rw mutex: add configurable priority
Dan Burkert has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Adar Dembo has submitted this change and it was merged. Change subject: rw_mutex: add configurable priority .. rw_mutex: add configurable priority The glibc implementation of pthread rwlocks exposes priorities that can help if avoiding reader or writer starvation is desirable. I have a use case for the latter, so let's expose the priorities in our wrapper. Note: pthread rwlock priorities don't exist on macOS, which is why they're a "best effort". Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Reviewed-on: http://gerrit.cloudera.org:8080/3603 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon Reviewed-by: Dan Burkert --- M src/kudu/util/rw_mutex.cc M src/kudu/util/rw_mutex.h 2 files changed, 54 insertions(+), 0 deletions(-) Approvals: Dan Burkert: Looks good to me, approved Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rw mutex: add configurable priority
Adar Dembo has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/3603/4/src/kudu/util/rw_mutex.h File src/kudu/util/rw_mutex.h: Line 28: // Implemented as a thin wrapper around pthread_rwlock_t. > A note about reentrance is warranted I think. I added one in the follow-on patch (the one that bans recursive acquisition). -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rw mutex: add configurable priority
Dan Burkert has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/3603/4/src/kudu/util/rw_mutex.h File src/kudu/util/rw_mutex.h: Line 28: // Implemented as a thin wrapper around pthread_rwlock_t. A note about reentrance is warranted I think. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rw mutex: add configurable priority
Todd Lipcon has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Kudu Jenkins has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2406/ -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3603 to look at the new patch set (#4). Change subject: rw_mutex: add configurable priority .. rw_mutex: add configurable priority The glibc implementation of pthread rwlocks exposes priorities that can help if avoiding reader or writer starvation is desirable. I have a use case for the latter, so let's expose the priorities in our wrapper. Note: pthread rwlock priorities don't exist on macOS, which is why they're a "best effort". Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 --- M src/kudu/util/rw_mutex.cc M src/kudu/util/rw_mutex.h 2 files changed, 54 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/3603/4 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rw mutex: add configurable priority
Adar Dembo has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 3: > I have experienced in the past an issue where the fairness policy > causes a deadlock. See > https://issues.apache.org/jira/browse/HDFS-2223?focusedCommentId=13097647&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13097647 > for example. > > I think the issue only happens when you recursively acquire the > read lock, or if you are holding the read lock while waiting on > another thread which needs to acquire the read lock. I'm not sure > if we have cases of either, but we should probably document this > danger. I see. I added a comment to the fairness policies to highlight the dangers. I've also got a follow-on patch that prohibits recursive locking altogether, so that should help too. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Todd Lipcon has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 3: I have experienced in the past an issue where the fairness policy causes a deadlock. See https://issues.apache.org/jira/browse/HDFS-2223?focusedCommentId=13097647&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13097647 for example. I think the issue only happens when you recursively acquire the read lock, or if you are holding the read lock while waiting on another thread which needs to acquire the read lock. I'm not sure if we have cases of either, but we should probably document this danger. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Dan Burkert has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Kudu Jenkins has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2320/ -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Adar Dembo has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 2: > Is there any possibility that the different priorities could create > a case where OSX would deadlock but Linux wouldn't? am slightly > worried about introducing different semantics on the different > platforms (and not just a perf difference) Lock priority can affect starvation or denial of service, but I don't see how it could affect deadlocks (which are, in my experience, "provable" one way or the other). If a particular application of the new PREFER_WRITING priority somehow prevented a deadlock, then before this patch that RWMutex was already deadlock prone and buggy. More concretely, I expect that with this patch, an application of PREFER_WRITING on Linux may show better performance or fewer timeouts when under stress than the same code on macOS. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Dan Burkert has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3603/1/src/kudu/util/rw_mutex.h File src/kudu/util/rw_mutex.h: Line 31: enum Priority { > Neat, wasn't aware of this feature. Unfortunately, it makes logging the enu Fair enough, but I don't think that default case is adding any safety. Because enum classes are not castable to int, it's not really feasible to receive an 'unknown' variant. If a variant is added later and the switch is not updated, the compiler will issue a warning. Line 57: void Init(Priority prio); > My understanding is that there's no way to "chain" constructors in C++, so ah ok I forgot that. Looks like c++11 has limited support that would probably cover this case, but this is enough of a niche feature it's probably not worth it. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rw mutex: add configurable priority
Todd Lipcon has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 2: Is there any possibility that the different priorities could create a case where OSX would deadlock but Linux wouldn't? am slightly worried about introducing different semantics on the different platforms (and not just a perf difference) -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Kudu Jenkins has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 2: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2300/ -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3603 to look at the new patch set (#2). Change subject: rw_mutex: add configurable priority .. rw_mutex: add configurable priority The glibc implementation of pthread rwlocks exposes priorities that can help if avoiding reader or writer starvation is desirable. I have a use case for the latter, so let's expose the priorities in our wrapper. Note: pthread rwlock priorities don't exist on macOS, which is why they're a "best effort". Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 --- M src/kudu/util/rw_mutex.cc M src/kudu/util/rw_mutex.h 2 files changed, 47 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/3603/2 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rw mutex: add configurable priority
Adar Dembo has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3603/1/src/kudu/util/rw_mutex.h File src/kudu/util/rw_mutex.h: Line 31: enum Priority { > consider making this an enum class. Neat, wasn't aware of this feature. Unfortunately, it makes logging the enum value in the case where it somehow isn't one these two values difficult: /home/adar/Source/kudu/src/kudu/util/rw_mutex.cc: In member function ‘void kudu::RWMutex::Init(kudu::RWMutex::Priority)’: /home/adar/Source/kudu/src/kudu/util/rw_mutex.cc:53:41: error: no match for ‘operator<<’ (operand types are ‘std::basic_ostream’ and ‘kudu::RWMutex::Priority’) LOG(FATAL) << "Unknown priority: " << prio; The solution proposed in http://stackoverflow.com/a/11421471 is pretty ugly, so unless you know of a better way, I'm going to keep the code as-is. Line 57: void Init(Priority prio); > any reason to have a separate init instead of doing it in the ctor as befor My understanding is that there's no way to "chain" constructors in C++, so I pulled the common initialization code into Init() and am invoking it from both constructors. -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rw mutex: add configurable priority
Dan Burkert has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3603/1/src/kudu/util/rw_mutex.h File src/kudu/util/rw_mutex.h: Line 31: enum Priority { consider making this an enum class. Line 57: void Init(Priority prio); any reason to have a separate init instead of doing it in the ctor as before? -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rw mutex: add configurable priority
Kudu Jenkins has posted comments on this change. Change subject: rw_mutex: add configurable priority .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2264/ -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rw mutex: add configurable priority
Hello Dan Burkert, David Ribeiro Alves, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/3603 to review the following change. Change subject: rw_mutex: add configurable priority .. rw_mutex: add configurable priority The glibc implementation of pthread rwlocks exposes priorities that can help if avoiding reader or writer starvation is desirable. I have a use case for the latter, so let's expose the priorities in our wrapper. Note: pthread rwlock priorities don't exist on macOS, which is why they're a "best effort". Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 --- M src/kudu/util/rw_mutex.cc M src/kudu/util/rw_mutex.h 2 files changed, 47 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/3603/1 -- To view, visit http://gerrit.cloudera.org:8080/3603 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I16ba6cd041f126c94e63fa07a1e84c88db6778d7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon