[kudu-CR] rw mutex: add configurable priority

[Adar Dembo (Code Review)]

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 (Code Review)]

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 (Code Review)]

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

[Adar Dembo (Code Review)]

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=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 (Code Review)]

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=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 (Code Review)]

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

[Adar Dembo (Code Review)]

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

[Todd Lipcon (Code Review)]

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 (Code Review)]

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

[Adar Dembo (Code Review)]

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 (Code Review)]

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

[Kudu Jenkins (Code Review)]

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

[Adar Dembo (Code Review)]

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