Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
623863002:
Implement pthread_rwlock functions for NaCl newlib. (Closed)
|
Created:
6 years, 2 months ago by Sam Clegg Modified:
5 years, 10 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionImplement pthread_rwlock functions for NaCl newlib.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3960
TEST=run_rwlock_test
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/9e3dac822315394238e650282a43fb4aacdbc6c9
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : spellcheck #
Total comments: 30
Patch Set 7 : #Patch Set 8 : #
Total comments: 32
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 1
Patch Set 12 : #
Total comments: 16
Patch Set 13 : #Patch Set 14 : #
Total comments: 6
Patch Set 15 : #
Messages
Total messages: 20 (2 generated)
sbc@chromium.org changed reviewers: + mseaborn@chromium.org
friendly ping.
On 2014/10/10 17:24:16, Sam Clegg wrote: > friendly ping. Thoughts?
On 2014/10/21 18:36:19, Sam Clegg wrote: > On 2014/10/10 17:24:16, Sam Clegg wrote: > > friendly ping. > > Thoughts? ping. Sorry about pestering you with all these reviews mark. If you are overloaded just let me know and I'll find other reviewers.
Big apologies for putting off reviewing this for so long. The basic approach looks OK. I don't fully understand why you switched to a pthread_mutex+cond-based version instead of using futexes directly (as in https://codereview.chromium.org/609363003/), but I assume you wanted to avoid potential subtle correctness problems from using atomics + futexes. A subtle issue is that I think rdlock/wrlock don't block as they're supposed to (see below). https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:10: * This implementation avoids writer starvation by preventing readers You might want to comment on whether/why you want to avoid writer starvation. It's not required by the standard, but might be desirable anyway, though might come at a performance cost. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:18: #include <assert.h> Not used https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:42: pthread_mutex_init(&rwlock->mutex, NULL); You should really check for errors from all the pthread calls, even if it's just to abort execution. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:48: /** Don't need "/**" here -- it's for doc comments, which wouldn't be needed for internal functions, and we're not supporting Doxygen anyway. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:55: * write lock is available if there is no current writer and no current Nit: capitalise as "Write" https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:72: * read lock is available if there is no current writer and no *waiting* Capitalise as "Read" too https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:107: struct timespec timeout = { 0, 0 }; This means a zero timeout, I think, so this won't block, which isn't what you want. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:111: int pthread_rwlock_tryrdlock(pthread_rwlock_t *rwlock) { You *could* merge this into rwlock_lock(), using a try_only flag to abort with EBUSY before calling pthread_cond_timedwait(). https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:162: /* Wait until there is no writer *and* no readers. */ Nit: trywrlock() doesn't wait https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:180: /* The write-lock is held. Ensure its us the holds it. */ "the" -> "that" https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:214: int pthread_rwlock_destroy(pthread_rwlock_t *rwlock) { Optional: You could check that reader_count and writers_waiting are both 0. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread.h (right): https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:179: Nit: add empty line to maintain spacing between sections https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:425: Nit: add empty line to maintain spacing between sections https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:19: #define READ_LOCK 1 Nit: use an enum? https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:212: Also test pthread_rwlock_destroy()?
Yes, I switched from futex/atomics to pthread primitives to make a simpler solution that was easier for me to reason about. If these are significant performance gains to be had from switching to futexes we can do that in a later optimization step. Since the other pthread primitives are futex based (at least on newlib and bionic) I doubt there will be significant gains, but even if there are I'd rather have a baseline implementation like this to start from. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:10: * This implementation avoids writer starvation by preventing readers On 2014/12/01 22:33:07, Mark Seaborn wrote: > You might want to comment on whether/why you want to avoid writer starvation. > It's not required by the standard, but might be desirable anyway, though might > come at a performance cost. I added a link to the wikipedia page with more info. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:18: #include <assert.h> On 2014/12/01 22:33:07, Mark Seaborn wrote: > Not used Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:42: pthread_mutex_init(&rwlock->mutex, NULL); On 2014/12/01 22:33:07, Mark Seaborn wrote: > You should really check for errors from all the pthread calls, even if it's just > to abort execution. Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:48: /** On 2014/12/01 22:33:07, Mark Seaborn wrote: > Don't need "/**" here -- it's for doc comments, which wouldn't be needed for > internal functions, and we're not supporting Doxygen anyway. Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:55: * write lock is available if there is no current writer and no current On 2014/12/01 22:33:07, Mark Seaborn wrote: > Nit: capitalise as "Write" Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:72: * read lock is available if there is no current writer and no *waiting* On 2014/12/01 22:33:07, Mark Seaborn wrote: > Capitalise as "Read" too Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:107: struct timespec timeout = { 0, 0 }; On 2014/12/01 22:33:07, Mark Seaborn wrote: > This means a zero timeout, I think, so this won't block, which isn't what you > want. Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:111: int pthread_rwlock_tryrdlock(pthread_rwlock_t *rwlock) { On 2014/12/01 22:33:07, Mark Seaborn wrote: > You *could* merge this into rwlock_lock(), using a try_only flag to abort with > EBUSY before calling pthread_cond_timedwait(). Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:162: /* Wait until there is no writer *and* no readers. */ On 2014/12/01 22:33:07, Mark Seaborn wrote: > Nit: trywrlock() doesn't wait Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:180: /* The write-lock is held. Ensure its us the holds it. */ On 2014/12/01 22:33:07, Mark Seaborn wrote: > "the" -> "that" Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:214: int pthread_rwlock_destroy(pthread_rwlock_t *rwlock) { On 2014/12/01 22:33:07, Mark Seaborn wrote: > Optional: You could check that reader_count and writers_waiting are both 0. Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread.h (right): https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:179: On 2014/12/01 22:33:07, Mark Seaborn wrote: > Nit: add empty line to maintain spacing between sections Done. https://codereview.chromium.org/623863002/diff/140001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:425: On 2014/12/01 22:33:07, Mark Seaborn wrote: > Nit: add empty line to maintain spacing between sections Done. https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:19: #define READ_LOCK 1 On 2014/12/01 22:33:07, Mark Seaborn wrote: > Nit: use an enum? Done. https://codereview.chromium.org/623863002/diff/140001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:212: On 2014/12/01 22:33:07, Mark Seaborn wrote: > Also test pthread_rwlock_destroy()? Done.
ping
https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:10: * This implementation is a 'write-prefering' reader-writer lock which "preferring" https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:13: * (http://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock) Nit: remove () brackets https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:21: #include <unistd.h> Is this used? https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:23: #include "native_client/src/include/nacl_compiler_annotations.h" Is this used? https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:24: #include "native_client/src/untrusted/nacl/nacl_irt.h" Nit: Not used, AFAICT https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:55: * Returns 1 if the write lock can be taken, 0 it it can't. "if it" https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:72: * Returns 1 if the write lock can be taken, 0 it it can't. Ditto https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:88: * This operates is three differnet ways in order to implement the three public "operates in", "different" https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:113: Remove empty line https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:130: * This operates is three differnet ways in order to implement the three public Same here https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:154: if (abs_timeout) { Style nit: Add "!= NULL" https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:171: pthread_mutex_unlock(&rwlock->mutex); You should really check the return value here too. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:207: /* The write lock is held. Ensure its us that holds it. */ "it's" "us" is vague. How about "Ensure it's the current thread that holds it"? https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:237: pthread_mutex_unlock(&rwlock->mutex); Ditto: check return value https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:242: /* Return EBUSY if anther thread hold the mutex */ "another", "holds". (Can you proof-read your comments, please?) https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread.h (right): https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:405: int pthread_rwlockattr_getpshared(const pthread_rwlockattr_t *attr, There's no implementation of get/setpshared in this change. Can you make the .h and .c files match up, please?
https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:10: * This implementation is a 'write-prefering' reader-writer lock which On 2015/01/05 16:58:53, Mark Seaborn wrote: > "preferring" Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:13: * (http://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock) On 2015/01/05 16:58:53, Mark Seaborn wrote: > Nit: remove () brackets Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:21: #include <unistd.h> On 2015/01/05 16:58:53, Mark Seaborn wrote: > Is this used? Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:23: #include "native_client/src/include/nacl_compiler_annotations.h" On 2015/01/05 16:58:53, Mark Seaborn wrote: > Is this used? Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:24: #include "native_client/src/untrusted/nacl/nacl_irt.h" On 2015/01/05 16:58:53, Mark Seaborn wrote: > Nit: Not used, AFAICT Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:55: * Returns 1 if the write lock can be taken, 0 it it can't. On 2015/01/05 16:58:53, Mark Seaborn wrote: > "if it" Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:72: * Returns 1 if the write lock can be taken, 0 it it can't. On 2015/01/05 16:58:53, Mark Seaborn wrote: > Ditto Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:88: * This operates is three differnet ways in order to implement the three public On 2015/01/05 16:58:53, Mark Seaborn wrote: > "operates in", "different" Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:113: On 2015/01/05 16:58:53, Mark Seaborn wrote: > Remove empty line Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:130: * This operates is three differnet ways in order to implement the three public On 2015/01/05 16:58:53, Mark Seaborn wrote: > Same here Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:154: if (abs_timeout) { On 2015/01/05 16:58:53, Mark Seaborn wrote: > Style nit: Add "!= NULL" Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:171: pthread_mutex_unlock(&rwlock->mutex); On 2015/01/05 16:58:53, Mark Seaborn wrote: > You should really check the return value here too. Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:207: /* The write lock is held. Ensure its us that holds it. */ On 2015/01/05 16:58:53, Mark Seaborn wrote: > "it's" > > "us" is vague. How about "Ensure it's the current thread that holds it"? Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:237: pthread_mutex_unlock(&rwlock->mutex); On 2015/01/05 16:58:53, Mark Seaborn wrote: > Ditto: check return value Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:242: /* Return EBUSY if anther thread hold the mutex */ On 2015/01/05 16:58:53, Mark Seaborn wrote: > "another", "holds". (Can you proof-read your comments, please?) Done. https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread.h (right): https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:405: int pthread_rwlockattr_getpshared(const pthread_rwlockattr_t *attr, On 2015/01/05 16:58:53, Mark Seaborn wrote: > There's no implementation of get/setpshared in this change. Can you make the .h > and .c files match up, please? Done. Add added test code.
On 2015/01/06 19:06:25, Sam Clegg wrote: > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > File src/untrusted/pthread/nc_rwlock.c (right): > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:10: * This implementation is a > 'write-prefering' reader-writer lock which > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > "preferring" > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:13: * > (http://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock) > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Nit: remove () brackets > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:21: #include <unistd.h> > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Is this used? > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:23: #include > "native_client/src/include/nacl_compiler_annotations.h" > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Is this used? > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:24: #include > "native_client/src/untrusted/nacl/nacl_irt.h" > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Nit: Not used, AFAICT > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:55: * Returns 1 if the write lock can be > taken, 0 it it can't. > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > "if it" > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:72: * Returns 1 if the write lock can be > taken, 0 it it can't. > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Ditto > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:88: * This operates is three differnet ways in > order to implement the three public > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > "operates in", "different" > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:113: > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Remove empty line > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:130: * This operates is three differnet ways > in order to implement the three public > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Same here > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:154: if (abs_timeout) { > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Style nit: Add "!= NULL" > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:171: pthread_mutex_unlock(&rwlock->mutex); > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > You should really check the return value here too. > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:207: /* The write lock is held. Ensure its us > that holds it. */ > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > "it's" > > > > "us" is vague. How about "Ensure it's the current thread that holds it"? > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:237: pthread_mutex_unlock(&rwlock->mutex); > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > Ditto: check return value > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:242: /* Return EBUSY if anther thread hold the > mutex */ > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > "another", "holds". (Can you proof-read your comments, please?) > > Done. > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... > File src/untrusted/pthread/pthread.h (right): > > https://codereview.chromium.org/623863002/diff/180001/src/untrusted/pthread/p... > src/untrusted/pthread/pthread.h:405: int pthread_rwlockattr_getpshared(const > pthread_rwlockattr_t *attr, > On 2015/01/05 16:58:53, Mark Seaborn wrote: > > There's no implementation of get/setpshared in this change. Can you make the > .h > > and .c files match up, please? > > Done. Add added test code. gentle ping
https://codereview.chromium.org/623863002/diff/240001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/240001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:93: if (rwlock->writers_waiting > 0) I was ready to LG this when I noticed a potentially-blocking problem. Plus I have a comment about a non-blocking issue. (Puns about blocking not intended. :-) ) 1) What happens if one thread claims a write lock while another thread recursively claims a read lock? For example: * Thread 1 calls pthread_rwlock_rdlock(): Claims read lock. * Thread 2 calls pthread_rwlock_rwlock(): It blocks. There is now a waiting writer. * Thread 1 calls pthread_rwlock_rdlock(): It blocks because there is a waiting writer. This causes a deadlock. Thread 1's recursive use of the lock is valid according to http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_rwlock_rdlock.html: "A thread may hold multiple concurrent read locks on rwlock (that is, successfully call the pthread_rwlock_rdlock() function n times). If so, the thread must perform matching unlocks (that is, it must call the pthread_rwlock_unlock() function n times)." I think this is why people usually don't implement pthread_rwlock_rdlock() as blocking when there are waiting writers. e.g. See this: http://stackoverflow.com/questions/2190090/how-to-prevent-writer-starvation-i... -- glibc provides PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP as an extension for programs that don't need recursive read locks. I expect this can be tweaked by changing read_lock_available() to return true if the read lock is currently held? 2) Non-blocker: The test coverage doesn't check correctness in some important cases. e.g. It doesn't check for the "zero timeout" problem I noticed earlier. I expect you probably don't want to add more tests at this stage, and this doesn't block committing this change. But I can give you some pointers on how to test properties like this, if you want.
On 2015/01/29 00:40:37, Mark Seaborn wrote: > https://codereview.chromium.org/623863002/diff/240001/src/untrusted/pthread/n... > File src/untrusted/pthread/nc_rwlock.c (right): > > https://codereview.chromium.org/623863002/diff/240001/src/untrusted/pthread/n... > src/untrusted/pthread/nc_rwlock.c:93: if (rwlock->writers_waiting > 0) > I was ready to LG this when I noticed a potentially-blocking problem. Plus I > have a comment about a non-blocking issue. (Puns about blocking not intended. > :-) ) > > 1) What happens if one thread claims a write lock while another thread > recursively claims a read lock? > > For example: > > * Thread 1 calls pthread_rwlock_rdlock(): Claims read lock. > * Thread 2 calls pthread_rwlock_rwlock(): It blocks. There is now a waiting > writer. > * Thread 1 calls pthread_rwlock_rdlock(): It blocks because there is a waiting > writer. This causes a deadlock. > > Thread 1's recursive use of the lock is valid according to > http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_rwlock_rdlock.html: "A > thread may hold multiple concurrent read locks on rwlock (that is, successfully > call the pthread_rwlock_rdlock() function n times). If so, the thread must > perform matching unlocks (that is, it must call the pthread_rwlock_unlock() > function n times)." > > I think this is why people usually don't implement pthread_rwlock_rdlock() as > blocking when there are waiting writers. > > e.g. See this: > http://stackoverflow.com/questions/2190090/how-to-prevent-writer-starvation-i... > -- glibc provides PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP as an extension > for programs that don't need recursive read locks. > > I expect this can be tweaked by changing read_lock_available() to return true if > the read lock is currently held? Good catch. Fixed in a conservative way as suggested in SO: blocking of readers to prevent writer starvation only applies to readers who don't hold any existing read locks. Added a test for this case too. > > 2) Non-blocker: The test coverage doesn't check correctness in some important > cases. e.g. It doesn't check for the "zero timeout" problem I noticed earlier. > I expect you probably don't want to add more tests at this stage, and this > doesn't block committing this change. But I can give you some pointers on how > to test properties like this, if you want. Ah, yes, we don't have any tests to verify that _rdlock()/_wrlock() actually block when they are supposed too, right? I'm open to adding such tests, and interested in your suggestions, but perhaps we can add TODO for them and land this initial version first?
https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:12: * while there are waiting writers. See: Add something like: "with an exception to prevent deadlocks in the case of recursive read locks (see read_lock_available())" https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:95: * is a waiting writer. However don't do this is the current thread "if the current thread" https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:100: if (rwlock->writers_waiting > 0 && pthread_self()->tdb->rdlock_count == 0) Optional: Using pthread_self()->tdb is suboptimal since it dereferences 2 pointers. It would be more efficient to move nc_get_tdb() to pthread_internal.h (and rename to __nc_get_tdb()) and use that. https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread_types.h (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/p... src/untrusted/pthread/pthread_types.h:55: int rdlock_count; /* how many rdlock's does this thread hold */ Nit: remove apostrophe. How about writing this as a noun phrase rather than a question: "number of rdlocks this thread holds" Also: * You need to initialise this field in nc_thread.c. * "unsigned" would be preferred to avoid undefined behaviour in the event of an overflow. https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:117: * Test that the an rdlock can be recursively aquired "acquired" Remove "the" https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:123: * Tell the locking thread to attempt to aquire the write lock. "acquire". Same below. https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:133: usleep(10 * 1000); Nit: check return value
https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:12: * while there are waiting writers. See: On 2015/02/02 18:29:13, Mark Seaborn wrote: > Add something like: "with an exception to prevent deadlocks in the case of > recursive read locks (see read_lock_available())" Done. https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:95: * is a waiting writer. However don't do this is the current thread On 2015/02/02 18:29:13, Mark Seaborn wrote: > "if the current thread" Done. https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:100: if (rwlock->writers_waiting > 0 && pthread_self()->tdb->rdlock_count == 0) On 2015/02/02 18:29:13, Mark Seaborn wrote: > Optional: Using pthread_self()->tdb is suboptimal since it dereferences 2 > pointers. > > It would be more efficient to move nc_get_tdb() to pthread_internal.h (and > rename to __nc_get_tdb()) and use that. Done. Did you want me to move the function itself into the header to keep it as inline? The only critical function that calls this is pthread_self().. we'd be adding function call overhead to there. https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread_types.h (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/p... src/untrusted/pthread/pthread_types.h:55: int rdlock_count; /* how many rdlock's does this thread hold */ On 2015/02/02 18:29:13, Mark Seaborn wrote: > Nit: remove apostrophe. > > How about writing this as a noun phrase rather than a question: "number of > rdlocks this thread holds" > > Also: > > * You need to initialise this field in nc_thread.c. > > * "unsigned" would be preferred to avoid undefined behaviour in the event of an > overflow. Done. https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:117: * Test that the an rdlock can be recursively aquired On 2015/02/02 18:29:13, Mark Seaborn wrote: > "acquired" > > Remove "the" Done. https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:123: * Tell the locking thread to attempt to aquire the write lock. On 2015/02/02 18:29:14, Mark Seaborn wrote: > "acquire". Same below. Done. https://codereview.chromium.org/623863002/diff/260001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:133: usleep(10 * 1000); On 2015/02/02 18:29:14, Mark Seaborn wrote: > Nit: check return value Done.
LGTM https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:100: if (rwlock->writers_waiting > 0 && pthread_self()->tdb->rdlock_count == 0) On 2015/02/02 19:25:08, Sam Clegg wrote: > On 2015/02/02 18:29:13, Mark Seaborn wrote: > > Optional: Using pthread_self()->tdb is suboptimal since it dereferences 2 > > pointers. > > > > It would be more efficient to move nc_get_tdb() to pthread_internal.h (and > > rename to __nc_get_tdb()) and use that. > > Done. Did you want me to move the function itself into the header to keep it > as inline? The only critical function that calls this is pthread_self().. we'd > be adding function call overhead to there. Either is OK, but I think inline in the header is slightly preferable. https://codereview.chromium.org/623863002/diff/300001/src/untrusted/pthread/p... File src/untrusted/pthread/pthread.h (right): https://codereview.chromium.org/623863002/diff/300001/src/untrusted/pthread/p... src/untrusted/pthread/pthread.h:140: struct __nc_basic_thread_data *writer_thread_id; Nit: slightly more efficient to use nc_thread_descriptor/__nc_get_tdb() for this field too (instead of pthread_self()). https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:14: static pthread_rwlock_t rwlock; Nit: add "g_" prefix to these globals https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:15: volatile int thread_has_lock = 0; Nit: be consistent about whether these globals are 'static' https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:51: lock_type == WRITE_LOCK ? "WRITE" : "READ" ); Nit: remove space before ")", and indent to align with "("
https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... File src/untrusted/pthread/nc_rwlock.c (right): https://codereview.chromium.org/623863002/diff/260001/src/untrusted/pthread/n... src/untrusted/pthread/nc_rwlock.c:100: if (rwlock->writers_waiting > 0 && pthread_self()->tdb->rdlock_count == 0) On 2015/02/02 20:52:24, Mark Seaborn wrote: > On 2015/02/02 19:25:08, Sam Clegg wrote: > > On 2015/02/02 18:29:13, Mark Seaborn wrote: > > > Optional: Using pthread_self()->tdb is suboptimal since it dereferences 2 > > > pointers. > > > > > > It would be more efficient to move nc_get_tdb() to pthread_internal.h (and > > > rename to __nc_get_tdb()) and use that. > > > > Done. Did you want me to move the function itself into the header to keep it > > as inline? The only critical function that calls this is pthread_self().. > we'd > > be adding function call overhead to there. > > Either is OK, but I think inline in the header is slightly preferable. Done. https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... File tests/threads/rwlock_test.c (right): https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:15: volatile int thread_has_lock = 0; On 2015/02/02 20:52:24, Mark Seaborn wrote: > Nit: be consistent about whether these globals are 'static' Done. https://codereview.chromium.org/623863002/diff/300001/tests/threads/rwlock_te... tests/threads/rwlock_test.c:51: lock_type == WRITE_LOCK ? "WRITE" : "READ" ); On 2015/02/02 20:52:24, Mark Seaborn wrote: > Nit: remove space before ")", and indent to align with "(" Done.
The CQ bit was checked by sbc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/623863002/320001
Message was sent while issue was closed.
Committed patchset #15 (id:320001) as https://chromium.googlesource.com/native_client/src/native_client/+/9e3dac822... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
