Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178503003:
Add PTHREAD_PRIO_INHERIT Locks for Mac (Closed)
DescriptionAdd PTHREAD_PRIO_INHERIT Locks for Mac
The Browser Task Scheduler will be using thread priorities.
PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread
priorities on Mac.
Linux currently cannot use them due to security concerns in the kernel priority
inheritance futex.
BUG=611856
Committed: https://crrev.com/206071838287c145e6a71f57744fd47bf24d614b
Cr-Commit-Position: refs/heads/master@{#409052}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR Feedback #
Total comments: 16
Patch Set 3 : CR Feedback #
Total comments: 2
Patch Set 4 : Adjust Macro Location #
Messages
Total messages: 51 (20 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: + danakj@chromium.org
danakj: Please review this CL. This will supersede https://codereview.chromium.org/2028303005/ Thanks!
drive-by https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:59: bool LockImpl::PriorityInheritanceAvailable() { There's no longer anything run-time about this method? Why not just have a single compile-time check? https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:60: #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() && defined(OS_MACOSX) Please add a comment explaining why it doesn't work on Linux (it took you so much effort to figure it out, we should at least encode the learnings in the code somewhere -- or link to an @chromium.org doc if it's too complicated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:59: bool LockImpl::PriorityInheritanceAvailable() { On 2016/07/22 20:07:31, gab wrote: > There's no longer anything run-time about this method? Why not just have a > single compile-time check? This future-proofs lock against the known runtime checks we'll need to do if/when we support Linux. https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:60: #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() && defined(OS_MACOSX) On 2016/07/22 20:07:31, gab wrote: > Please add a comment explaining why it doesn't work on Linux (it took you so > much effort to figure it out, we should at least encode the learnings in the > code somewhere -- or link to an @chromium.org doc if it's too complicated. Done.
gab@chromium.org changed reviewers: + gab@chromium.org
lgtm https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:59: bool LockImpl::PriorityInheritanceAvailable() { On 2016/07/25 20:53:19, robliao wrote: > On 2016/07/22 20:07:31, gab wrote: > > There's no longer anything run-time about this method? Why not just have a > > single compile-time check? > > This future-proofs lock against the known runtime checks we'll need to do > if/when we support Linux. Ok, can you add a TODO here then to indicate the intent of this method? (without it, first thing I would do as a reader going through here is refactor them in one call)
lgtm https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/1/base/synchronization/lock_i... base/synchronization/lock_impl_posix.cc:59: bool LockImpl::PriorityInheritanceAvailable() { On 2016/07/26 17:31:27, gab wrote: > On 2016/07/25 20:53:19, robliao wrote: > > On 2016/07/22 20:07:31, gab wrote: > > > There's no longer anything run-time about this method? Why not just have a > > > single compile-time check? > > > > This future-proofs lock against the known runtime checks we'll need to do > > if/when we support Linux. > > Ok, can you add a TODO here then to indicate the intent of this method? (without > it, first thing I would do as a reader going through here is refactor them in > one call) Nvm, see you addressed this already with other comment, thanks!
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping for danakj. Thanks!
https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:17: #if defined(OS_POSIX) Why is this behind OS_POSIX? I'd expect it to just be 0 on non-posix? https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:80: #if defined(OS_POSIX) And why is this behind OS_POSIX instead of behind PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl.h:51: #if defined(OS_POSIX) ditto https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:20: #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() Alternatively, if guarding everything else by OS_POSIX, why is this guarding the call with this here, instead of just calling and getting false? https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:24: DCHECK_EQ(rv, 0) << ". " << strerror(rv); put this inside the if?
fdoray@chromium.org changed reviewers: + fdoray@chromium.org
https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:85: } Could this be available on all platforms and called something like IsProtectedAgainstPriorityInversion() or CanBeUsedWithMultipleThreadPriorities()? Then we wouldn't need an #if + a call to this method to know whether we can use thread priorities in base/task_scheduler. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:49: int rv = pthread_mutex_lock(&native_handle_); In DCHECK_IS_ON() builds, we could store the priority of the thread that created the lock and DCHECK(PriorityInherithanceAvailable() || thread_priority_ == PlatformThread::GetCurrentThreadPriority());
https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:17: #if defined(OS_POSIX) On 2016/07/27 22:47:53, danakj wrote: > Why is this behind OS_POSIX? I'd expect it to just be 0 on non-posix? The intention was to only have this in POSIX since it only impacts POSIX platforms. Windows platforms don't have such an option, so they really shouldn't be checking this. I've updated the comment to emphasis the compile time nature for this flag. (e.g. priority inheritance code may not compile on all POSIX platforms). https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:80: #if defined(OS_POSIX) On 2016/07/27 22:47:53, danakj wrote: > And why is this behind OS_POSIX instead of behind > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? See above. This is intended to be the runtime check. Code that is universally compileable (no dependency on pthreads) can use this runtime check. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:85: } On 2016/07/28 14:38:36, fdoray wrote: > Could this be available on all platforms and called something like > IsProtectedAgainstPriorityInversion() or > CanBeUsedWithMultipleThreadPriorities()? Then we wouldn't need an #if + a call > to this method to know whether we can use thread priorities in > base/task_scheduler. The #if is still necessary if code won't compile. (Both the sandbox and lock impl ran into this due to non-uniform pthreads headers). Since the complication is only on POSIX, I'd prefer to limit the check to POSIX. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl.h:51: #if defined(OS_POSIX) On 2016/07/27 22:47:53, danakj wrote: > ditto See previous comment. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock_impl_posix.cc (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:20: #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() On 2016/07/27 22:47:53, danakj wrote: > Alternatively, if guarding everything else by OS_POSIX, why is this guarding the > call with this here, instead of just calling and getting false? We're already in a POSIX-only file, so OS_POSIX should be guaranteed, shouldn't it? This code will fail to compile on NaCl and Android builds since PTHREAD_PRIO_INHERIT isn't defined in their pthreads headers. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:24: DCHECK_EQ(rv, 0) << ". " << strerror(rv); On 2016/07/27 22:47:53, danakj wrote: > put this inside the if? Done. https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock_impl_posix.cc:49: int rv = pthread_mutex_lock(&native_handle_); On 2016/07/28 14:38:36, fdoray wrote: > In DCHECK_IS_ON() builds, we could store the priority of the thread that created > the lock and DCHECK(PriorityInherithanceAvailable() || thread_priority_ == > PlatformThread::GetCurrentThreadPriority()); That's an interesting idea. I'd like to pursue that as a separate CL.
https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:17: #if defined(OS_POSIX) On 2016/07/28 20:06:09, robliao wrote: > On 2016/07/27 22:47:53, danakj wrote: > > Why is this behind OS_POSIX? I'd expect it to just be 0 on non-posix? > > The intention was to only have this in POSIX since it only impacts POSIX > platforms. Windows platforms don't have such an option, so they really shouldn't > be checking this. > > I've updated the comment to emphasis the compile time nature for this flag. > (e.g. priority inheritance code may not compile on all POSIX platforms). I see. It's just that to check this now you will have to do #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() But it'd be simpler to just #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() and it's always false on posix?
https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... base/synchronization/lock.h:17: #if defined(OS_POSIX) On 2016/07/28 20:49:03, danakj wrote: > On 2016/07/28 20:06:09, robliao wrote: > > On 2016/07/27 22:47:53, danakj wrote: > > > Why is this behind OS_POSIX? I'd expect it to just be 0 on non-posix? > > > > The intention was to only have this in POSIX since it only impacts POSIX > > platforms. Windows platforms don't have such an option, so they really > shouldn't > > be checking this. > > > > I've updated the comment to emphasis the compile time nature for this flag. > > (e.g. priority inheritance code may not compile on all POSIX platforms). > > I see. It's just that to check this now you will have to do > > #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > But it'd be simpler to just > > #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > and it's always false on posix? My sense is that code will fall under one of two categories: 1) Code in a *_posix* file The OS_POSIX check isn't necessary and you'd use PRIORITY_INHERITANCE_LOCKS_POSSIBLE directly. 2) Code in a common file You'd want to use OS_POSIX to make sure you don't get included in the future on some other platform. Is there something I'm missing?
On Thu, Jul 28, 2016 at 2:08 PM, <robliao@chromium.org> wrote: > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > File base/synchronization/lock.h (right): > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > base/synchronization/lock.h:17: #if defined(OS_POSIX) > On 2016/07/28 20:49:03, danakj wrote: > > On 2016/07/28 20:06:09, robliao wrote: > > > On 2016/07/27 22:47:53, danakj wrote: > > > > Why is this behind OS_POSIX? I'd expect it to just be 0 on > non-posix? > > > > > > The intention was to only have this in POSIX since it only impacts > POSIX > > > platforms. Windows platforms don't have such an option, so they > really > > shouldn't > > > be checking this. > > > > > > I've updated the comment to emphasis the compile time nature for > this flag. > > > (e.g. priority inheritance code may not compile on all POSIX > platforms). > > > > I see. It's just that to check this now you will have to do > > > > #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > But it'd be simpler to just > > > > #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > and it's always false on posix? > > My sense is that code will fall under one of two categories: > > 1) Code in a *_posix* file > The OS_POSIX check isn't necessary and you'd use > PRIORITY_INHERITANCE_LOCKS_POSSIBLE directly. > > 2) Code in a common file > You'd want to use OS_POSIX to make sure you don't get included in the > future on some other platform. > I guess I don't understand this case. Why would you want to ensure that? If PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown platform, you wouldn't want to have to go and change all the #if POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() on other platforms? > > Is there something I'm missing? > > https://codereview.chromium.org/2178503003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/07/28 21:14:27, danakj wrote: > On Thu, Jul 28, 2016 at 2:08 PM, <mailto:robliao@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > > File base/synchronization/lock.h (right): > > > > > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > > base/synchronization/lock.h:17: #if defined(OS_POSIX) > > On 2016/07/28 20:49:03, danakj wrote: > > > On 2016/07/28 20:06:09, robliao wrote: > > > > On 2016/07/27 22:47:53, danakj wrote: > > > > > Why is this behind OS_POSIX? I'd expect it to just be 0 on > > non-posix? > > > > > > > > The intention was to only have this in POSIX since it only impacts > > POSIX > > > > platforms. Windows platforms don't have such an option, so they > > really > > > shouldn't > > > > be checking this. > > > > > > > > I've updated the comment to emphasis the compile time nature for > > this flag. > > > > (e.g. priority inheritance code may not compile on all POSIX > > platforms). > > > > > > I see. It's just that to check this now you will have to do > > > > > > #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > > > But it'd be simpler to just > > > > > > #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > > > and it's always false on posix? > > > > My sense is that code will fall under one of two categories: > > > > 1) Code in a *_posix* file > > The OS_POSIX check isn't necessary and you'd use > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE directly. > > > > 2) Code in a common file > > You'd want to use OS_POSIX to make sure you don't get included in the > > future on some other platform. > > > > I guess I don't understand this case. Why would you want to ensure that? If > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown > platform, you wouldn't want to have to go and change all the #if POSIX && > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? > > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() on > other platforms? > > > > > > Is there something I'm missing? > > > > https://codereview.chromium.org/2178503003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I guess I don't understand this case. Why would you want to ensure that? If > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown > platform, you wouldn't want to have to go and change all the #if POSIX && > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() on > other platforms? The primary reason for PRIORITY_INHERITANCE_LOCKS_POSSIBLE is to address the non-uniformity of POSIX pthread headers. Since PTHREAD_PRIO_INHERIT isn't defined on Android and NaCl, POSIX code is no longer write-once and compile-anywhere on all POSIX platforms. PRIORITY_INHERITANCE_LOCKS_POSSIBLE is for folks who are writing this platform specific code (locks and sandbox). PRIORITY_INHERITANCE_LOCKS_POSSIBLE is also a necessary but insufficient check for priority inheritance locks due to the bugs that need to be detected at runtime on Linux, hence the existence of PriorityInheritanceAvailable. POSIX code dealing with thread priorities will be the only code that needs to deal with this quirk. Windows code is generally written in a backward compatible way, so any check is unlikely to need PRIORITY_INHERITANCE_LOCKS_POSSIBLE. A future platform that gets introduced could have these quirks. However, since platform code has a high probability of living between PRIORITY_INHERITANCE_LOCKS_POSSIBLE due to the compile issues, then it's probable that we would need to introduce OS_POSIX checks everywhere for this code. There is an argument to be made that maybe PriorityInheritanceAvailable should live on all locks. On Windows it would likely be set to return true, even though priority inheritance doesn't truly exist, so it didn't make sense to me to have this be available on Windows.
On Thu, Jul 28, 2016 at 2:52 PM, <robliao@chromium.org> wrote: > On 2016/07/28 21:14:27, danakj wrote: > > > On Thu, Jul 28, 2016 at 2:08 PM, <mailto:robliao@chromium.org> wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > > > File base/synchronization/lock.h (right): > > > > > > > > > > > > > https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... > > > base/synchronization/lock.h:17: #if defined(OS_POSIX) > > > On 2016/07/28 20:49:03, danakj wrote: > > > > On 2016/07/28 20:06:09, robliao wrote: > > > > > On 2016/07/27 22:47:53, danakj wrote: > > > > > > Why is this behind OS_POSIX? I'd expect it to just be 0 on > > > non-posix? > > > > > > > > > > The intention was to only have this in POSIX since it only impacts > > > POSIX > > > > > platforms. Windows platforms don't have such an option, so they > > > really > > > > shouldn't > > > > > be checking this. > > > > > > > > > > I've updated the comment to emphasis the compile time nature for > > > this flag. > > > > > (e.g. priority inheritance code may not compile on all POSIX > > > platforms). > > > > > > > > I see. It's just that to check this now you will have to do > > > > > > > > #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > > > > > But it'd be simpler to just > > > > > > > > #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > > > > > > > > and it's always false on posix? > > > > > > My sense is that code will fall under one of two categories: > > > > > > 1) Code in a *_posix* file > > > The OS_POSIX check isn't necessary and you'd use > > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE directly. > > > > > > 2) Code in a common file > > > You'd want to use OS_POSIX to make sure you don't get included in the > > > future on some other platform. > > > > > > > I guess I don't understand this case. Why would you want to ensure that? > If > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown > > platform, you wouldn't want to have to go and change all the #if POSIX && > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? > > > > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > on > > other platforms? > > > > > > > > > > Is there something I'm missing? > > > > > > https://codereview.chromium.org/2178503003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > I guess I don't understand this case. Why would you want to ensure that? > If > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown > > platform, you wouldn't want to have to go and change all the #if POSIX && > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? > > > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() > on > > other platforms? > > The primary reason for PRIORITY_INHERITANCE_LOCKS_POSSIBLE is to address > the > non-uniformity of POSIX pthread headers. > Since PTHREAD_PRIO_INHERIT isn't defined on Android and NaCl, POSIX code > is no > longer write-once and compile-anywhere on all POSIX platforms. > PRIORITY_INHERITANCE_LOCKS_POSSIBLE is for folks who are writing this > platform > specific code (locks and sandbox). > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE is also a necessary but insufficient > check > for priority inheritance locks due to the bugs that need to be detected at > runtime on Linux, hence the existence of PriorityInheritanceAvailable. > > POSIX code dealing with thread priorities will be the only code that needs > to > deal with this quirk. > Is there anywhere other than lock_impl_posix that will need this at compile time? It doesn't seem to me that anyone else would need to use PTHREAD_PRIO_INHERIT itself. Maybe linux sandbox code from looking at https://codereview.chromium.org/2028303005/ again, though that is uncertain, and not the case in this CL. Are there going to be callers to PriorityInheritanceAvailable() outside of lock_impl_posix.cc? I guess I am wondering why any of this needs to be part of the public API right now. > > Windows code is generally written in a backward compatible way, so any > check is > unlikely to need PRIORITY_INHERITANCE_LOCKS_POSSIBLE. > A future platform that gets introduced could have these quirks. However, > since > platform code has a high probability of living between > PRIORITY_INHERITANCE_LOCKS_POSSIBLE due to the compile issues, then it's > probable that we would need to introduce OS_POSIX checks everywhere for > this > code. > > > > There is an argument to be made that maybe PriorityInheritanceAvailable > should > live on all locks. On Windows it would likely be set to return true, even > though > priority inheritance doesn't truly exist, so it didn't make sense to me to > have > this be available on Windows. > > https://codereview.chromium.org/2178503003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yep. That is the required sandbox code change. The task scheduler will be checking for PriorityInheritanceAvailable to determine if it can turn on priorities for threads. On Thu, Jul 28, 2016 at 4:31 PM, <danakj@chromium.org> wrote: > On Thu, Jul 28, 2016 at 2:52 PM, <robliao@chromium.org> wrote: > >> On 2016/07/28 21:14:27, danakj wrote: >> >> > On Thu, Jul 28, 2016 at 2:08 PM, <mailto:robliao@chromium.org> wrote: >> > >> > > >> > > >> > > >> > >> >> https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... >> > > File base/synchronization/lock.h (right): >> > > >> > > >> > > >> > >> >> https://codereview.chromium.org/2178503003/diff/20001/base/synchronization/lo... >> > > base/synchronization/lock.h:17: #if defined(OS_POSIX) >> > > On 2016/07/28 20:49:03, danakj wrote: >> > > > On 2016/07/28 20:06:09, robliao wrote: >> > > > > On 2016/07/27 22:47:53, danakj wrote: >> > > > > > Why is this behind OS_POSIX? I'd expect it to just be 0 on >> > > non-posix? >> > > > > >> > > > > The intention was to only have this in POSIX since it only impacts >> > > POSIX >> > > > > platforms. Windows platforms don't have such an option, so they >> > > really >> > > > shouldn't >> > > > > be checking this. >> > > > > >> > > > > I've updated the comment to emphasis the compile time nature for >> > > this flag. >> > > > > (e.g. priority inheritance code may not compile on all POSIX >> > > platforms). >> > > > >> > > > I see. It's just that to check this now you will have to do >> > > > >> > > > #if OS_POSIX && PRIORITY_INHERITANCE_LOCKS_POSSIBLE() >> > > > >> > > > But it'd be simpler to just >> > > > >> > > > #if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() >> > > > >> > > > and it's always false on posix? >> > > >> > > My sense is that code will fall under one of two categories: >> > > >> > > 1) Code in a *_posix* file >> > > The OS_POSIX check isn't necessary and you'd use >> > > PRIORITY_INHERITANCE_LOCKS_POSSIBLE directly. >> > > >> > > 2) Code in a common file >> > > You'd want to use OS_POSIX to make sure you don't get included in the >> > > future on some other platform. >> > > >> > >> > I guess I don't understand this case. Why would you want to ensure >> that? If >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown >> > platform, you wouldn't want to have to go and change all the #if POSIX >> && >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? >> > >> > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() >> on >> > other platforms? >> > >> > >> > > >> > > Is there something I'm missing? >> > > >> > > https://codereview.chromium.org/2178503003/ >> > > >> > >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> > I guess I don't understand this case. Why would you want to ensure >> that? If >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE() became true on some future unknown >> > platform, you wouldn't want to have to go and change all the #if POSIX >> && >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE to be #if (POSIX || THING) && >> > PRIORITY_INHERITANCE_LOCKS_POSSIBLE()? >> >> > Why do you want to avoid checking PRIORITY_INHERITANCE_LOCKS_POSSIBLE() >> on >> > other platforms? >> >> The primary reason for PRIORITY_INHERITANCE_LOCKS_POSSIBLE is to address >> the >> non-uniformity of POSIX pthread headers. >> Since PTHREAD_PRIO_INHERIT isn't defined on Android and NaCl, POSIX code >> is no >> longer write-once and compile-anywhere on all POSIX platforms. >> PRIORITY_INHERITANCE_LOCKS_POSSIBLE is for folks who are writing this >> platform >> specific code (locks and sandbox). >> >> PRIORITY_INHERITANCE_LOCKS_POSSIBLE is also a necessary but insufficient >> check >> for priority inheritance locks due to the bugs that need to be detected at >> runtime on Linux, hence the existence of PriorityInheritanceAvailable. >> >> POSIX code dealing with thread priorities will be the only code that >> needs to >> deal with this quirk. >> > > Is there anywhere other than lock_impl_posix that will need this at > compile time? It doesn't seem to me that anyone else would need to > use PTHREAD_PRIO_INHERIT itself. Maybe linux sandbox code from looking at > https://codereview.chromium.org/2028303005/ again, though that is > uncertain, and not the case in this CL. > > Are there going to be callers to PriorityInheritanceAvailable() outside of > lock_impl_posix.cc? I guess I am wondering why any of this needs to be part > of the public API right now. > > >> >> Windows code is generally written in a backward compatible way, so any >> check is >> unlikely to need PRIORITY_INHERITANCE_LOCKS_POSSIBLE. >> A future platform that gets introduced could have these quirks. However, >> since >> platform code has a high probability of living between >> PRIORITY_INHERITANCE_LOCKS_POSSIBLE due to the compile issues, then it's >> probable that we would need to introduce OS_POSIX checks everywhere for >> this >> code. >> >> >> >> There is an argument to be made that maybe PriorityInheritanceAvailable >> should >> live on all locks. On Windows it would likely be set to return true, even >> though >> priority inheritance doesn't truly exist, so it didn't make sense to me >> to have >> this be available on Windows. >> >> https://codereview.chromium.org/2178503003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
FWIW (I didn't read every single word of your conversation but think the essence was about why the checks were behind OS_POSIX), I agree it would be nice to have a method/macro available on every platform as it would allow us to document in code comments why we consider this safe on Windows even though there isn't SCHED_PRIO_INHERIT there.
On 2016/07/29 13:25:07, gab wrote: > FWIW (I didn't read every single word of your conversation but think the essence > was about why the checks were behind OS_POSIX), I agree it would be nice to have > a method/macro available on every platform as it would allow us to document in > code comments why we consider this safe on Windows even though there isn't > SCHED_PRIO_INHERIT there. I would rename PriorityInheritanceAvailable() to CanBeUsedWithDifferentThreadPriorities() and make it available on all platforms. That would make the check in base/task_scheduler very simple. I think CanBeUsedWithDifferentThreadPriorities is an accurate name because Windows locks "can be used with different thread priorities" thanks to random priority bumping while POSIX locks "can be used with different thread priorities" when priority inheritance is available. The macro can stay but I wouldn't expect it to be called outside of sandbox and lock_impl_posix.cc code.
On 2016/07/29 15:05:41, fdoray wrote: > On 2016/07/29 13:25:07, gab wrote: > > FWIW (I didn't read every single word of your conversation but think the > essence > > was about why the checks were behind OS_POSIX), I agree it would be nice to > have > > a method/macro available on every platform as it would allow us to document in > > code comments why we consider this safe on Windows even though there isn't > > SCHED_PRIO_INHERIT there. > > I would rename PriorityInheritanceAvailable() to > CanBeUsedWithDifferentThreadPriorities() and make it available on all platforms. > That would make the check in base/task_scheduler very simple. I think > CanBeUsedWithDifferentThreadPriorities is an accurate name because Windows locks > "can be used with different thread priorities" thanks to random priority bumping > while POSIX locks "can be used with different thread priorities" when priority > inheritance is available. > > The macro can stay but I wouldn't expect it to be called outside of sandbox and > lock_impl_posix.cc code. The intention behind naming it this way is to keep it as a characteristic (the lock will or will not use priority inheritance) and avoid guidance of policy (others can immediately use this value to determine if different priorities should be used). The guidance for that could go beyond just the lock. This suggests that a policy guidance for this should exist at the thread level rather than at the lock level. That would be available on all platforms.
On 2016/07/29 18:17:37, robliao wrote: > On 2016/07/29 15:05:41, fdoray wrote: > > On 2016/07/29 13:25:07, gab wrote: > > > FWIW (I didn't read every single word of your conversation but think the > > essence > > > was about why the checks were behind OS_POSIX), I agree it would be nice to > > have > > > a method/macro available on every platform as it would allow us to document > in > > > code comments why we consider this safe on Windows even though there isn't > > > SCHED_PRIO_INHERIT there. > > > > I would rename PriorityInheritanceAvailable() to > > CanBeUsedWithDifferentThreadPriorities() and make it available on all > platforms. > > That would make the check in base/task_scheduler very simple. I think > > CanBeUsedWithDifferentThreadPriorities is an accurate name because Windows > locks > > "can be used with different thread priorities" thanks to random priority > bumping > > while POSIX locks "can be used with different thread priorities" when priority > > inheritance is available. > > > > The macro can stay but I wouldn't expect it to be called outside of sandbox > and > > lock_impl_posix.cc code. > > The intention behind naming it this way is to keep it as a characteristic (the > lock will or will not use priority inheritance) and avoid guidance of policy > (others can immediately use this value to determine if different priorities > should be used). I understand your point of view. I would have liked an interface that provides guidance about the context in which it can be used (+ an impl that DCHECKs on that) without exposing the concept of priority inheritance. Windows uses priority bumping to solve the same problem as priority inheritance and I would like if users didn't have to worry about that. > The guidance for that could go beyond just the lock. This suggests that a policy > guidance for this should exist at the thread level rather than at the lock > level. That would be available on all platforms. The fact that priority inheritance isn't available should prevent the use of a lock from different thread priorities, not the creation of threads with different priorities. lgtm
Another thought.. what if PriorityInheritanceAvailable() is constexpr. Would we need the macro then?
On 2016/07/29 20:39:52, danakj wrote: > Another thought.. what if PriorityInheritanceAvailable() is constexpr. Would we > need the macro then? PriorityInheritanceAvailable isn't constexpr on Linux platforms (runtime check required for glibc). Does a branched constexpr expression get removed from the build? That's the main issue solved by the macro.
On 2016/07/29 20:42:38, robliao wrote:
> On 2016/07/29 20:39:52, danakj wrote:
> > Another thought.. what if PriorityInheritanceAvailable() is constexpr. Would
> we
> > need the macro then?
>
> PriorityInheritanceAvailable isn't constexpr on Linux platforms (runtime check
> required for glibc).
>
> Does a branched constexpr expression get removed from the build? That's the
main
> issue solved by the macro.
To answer my own question, constexpr'ed branches are compiled as part of the
build.
constexpr bool AlwaysReturnTrue() {
return true;
}
int main(int argc, char* argv[]) {
if (!AlwaysReturnTrue()) {
UndefinedFunction();
}
return 0;
}
I guess it's still up to the optimizer to remove the branch, so it still needs
to be compilable.
On Fri, Jul 29, 2016 at 1:42 PM, <robliao@chromium.org> wrote: > On 2016/07/29 20:39:52, danakj wrote: > > Another thought.. what if PriorityInheritanceAvailable() is constexpr. > Would > we > > need the macro then? > > PriorityInheritanceAvailable isn't constexpr on Linux platforms (runtime > check > required for glibc). > Oh, gnu_get_libc_version()? I was forgetting that and expecting something at compile time like: https://cs.chromium.org/chromium/src/base/template_util.h?rcl=0&l=17 > Does a branched constexpr expression get removed from the build? That's > the main > issue solved by the macro. > I think you'd use templates then to do things at compile time instead of if(). template<bool partial> struct Stuff; template<> struct Stuff<true> { void DoStuff() { .. } }; template<> struct Stuff<false> { void DoStuff() { .. } }; Stuff<Lock::InheritanceOk()>::DoStuff(). Modulo writing templates without compiling them is impossible and my syntax is surely wrong. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Agreed on the trickyness of templates without a compiler :-). The code in question does involve gnu_get_libc_version: // glibc Bug 14652: https://sourceware.org/bugzilla/show_bug.cgi?id=14652 // Fixed in glibc 2.17. // Priority inheritance mutexes may deadlock with condition variables during // recacquisition of the mutex after the condition variable is signalled. Version version(gnu_get_libc_version()); if (!version.IsValid() || version.components().size() != 2) return false; return std::tie(version.components()[0], version.components()[1]) >= std::make_tuple(2, 17); The usage seems to indicate that different glibc versions have different headers. The issue for priority inheritance is that the actual glibc binary has a bug at 2.17 or lower. We encounter this on Ubuntu 12.04, used by the bots. On Fri, Jul 29, 2016 at 1:54 PM, <danakj@chromium.org> wrote: > On Fri, Jul 29, 2016 at 1:42 PM, <robliao@chromium.org> wrote: > >> On 2016/07/29 20:39:52, danakj wrote: >> > Another thought.. what if PriorityInheritanceAvailable() is constexpr. >> Would >> we >> > need the macro then? >> >> PriorityInheritanceAvailable isn't constexpr on Linux platforms (runtime >> check >> required for glibc). >> > > Oh, gnu_get_libc_version()? I was forgetting that and expecting something > at compile time like: > https://cs.chromium.org/chromium/src/base/template_util.h?rcl=0&l=17 > > >> Does a branched constexpr expression get removed from the build? That's >> the main >> issue solved by the macro. >> > > I think you'd use templates then to do things at compile time instead of > if(). > > template<bool partial> struct Stuff; > template<> struct Stuff<true> { > void DoStuff() { .. } > }; > template<> struct Stuff<false> { > void DoStuff() { .. } > }; > > Stuff<Lock::InheritanceOk()>::DoStuff(). > > Modulo writing templates without compiling them is impossible and my > syntax is surely wrong. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jul 29, 2016 at 2:01 PM, Robert Liao <robliao@chromium.org> wrote: > Agreed on the trickyness of templates without a compiler :-). > > The code in question does involve gnu_get_libc_version: > > // glibc Bug 14652: https://sourceware.org/bugzilla/show_bug.cgi?id=14652 > // Fixed in glibc 2.17. > // Priority inheritance mutexes may deadlock with condition variables during > // recacquisition of the mutex after the condition variable is signalled. > Version version(gnu_get_libc_version()); > if (!version.IsValid() || version.components().size() != 2) > return false; > > return std::tie(version.components()[0], version.components()[1]) >= > std::make_tuple(2, 17); > > The usage seems to indicate that different glibc versions have different headers. The issue for priority inheritance is that the actual glibc binary has a bug at 2.17 or lower. We encounter this on Ubuntu 12.04, used by the bots. > > Ah right, glibc != libstdc++ :) > > On Fri, Jul 29, 2016 at 1:54 PM, <danakj@chromium.org> wrote: > >> On Fri, Jul 29, 2016 at 1:42 PM, <robliao@chromium.org> wrote: >> >>> On 2016/07/29 20:39:52, danakj wrote: >>> > Another thought.. what if PriorityInheritanceAvailable() is constexpr. >>> Would >>> we >>> > need the macro then? >>> >>> PriorityInheritanceAvailable isn't constexpr on Linux platforms (runtime >>> check >>> required for glibc). >>> >> >> Oh, gnu_get_libc_version()? I was forgetting that and expecting something >> at compile time like: >> https://cs.chromium.org/chromium/src/base/template_util.h?rcl=0&l=17 >> >> >>> Does a branched constexpr expression get removed from the build? That's >>> the main >>> issue solved by the macro. >>> >> >> I think you'd use templates then to do things at compile time instead of >> if(). >> >> template<bool partial> struct Stuff; >> template<> struct Stuff<true> { >> void DoStuff() { .. } >> }; >> template<> struct Stuff<false> { >> void DoStuff() { .. } >> }; >> >> Stuff<Lock::InheritanceOk()>::DoStuff(). >> >> Modulo writing templates without compiling them is impossible and my >> syntax is surely wrong. >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2178503003/diff/40001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/40001/base/synchronization/lo... base/synchronization/lock.h:19: // Determines which platforms can consider using priority inheritance locks. Use OK my only nit is that I rather this lives in lock_impl_posix.cc and is an implementation detail until we need it outside. LGTM otherwise.
https://codereview.chromium.org/2178503003/diff/40001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2178503003/diff/40001/base/synchronization/lo... base/synchronization/lock.h:19: // Determines which platforms can consider using priority inheritance locks. Use On 2016/07/29 21:06:40, danakj wrote: > OK my only nit is that I rather this lives in lock_impl_posix.cc and is an > implementation detail until we need it outside. LGTM otherwise. sgtm. Moved and adjusted the comment for the more local scope.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, danakj@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2178503003/#ps60001 (title: "Adjust Macro Location")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add PTHREAD_PRIO_INHERIT Locks for Mac The Browser Task Scheduler will be using thread priorities. PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread priorities on Mac. Linux currently cannot use them due to security concerns in the kernel priority inheritance futex. BUG=611856 ========== to ========== Add PTHREAD_PRIO_INHERIT Locks for Mac The Browser Task Scheduler will be using thread priorities. PTHREAD_PRIO_INHERIT will help avoid priority inversion with the use of thread priorities on Mac. Linux currently cannot use them due to security concerns in the kernel priority inheritance futex. BUG=611856 Committed: https://crrev.com/206071838287c145e6a71f57744fd47bf24d614b Cr-Commit-Position: refs/heads/master@{#409052} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/206071838287c145e6a71f57744fd47bf24d614b Cr-Commit-Position: refs/heads/master@{#409052} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
