Consolidate PartitionAlloc TLS into a single structure. [chromium/src : main]
Mikihito Matsuura (Gerrit)
Mikihito Matsuura voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mikihito Matsuura
| Commit-Queue | +1 |
6 comments
GetThreadCache(size_details.bucket_index);Passing `size_details.bucket_index` here is likely a mistake because it restricts the ThreadCache lookup based on the size of the allocation. The `SchedulerLoopQuarantine` branch has its own size limits and is completely decoupled from the ThreadCache bucket limits.\n\nIn `Free()` (line 710), you are correctly using `GetThreadCache()` (which defaults to bucket 0) to get the quarantine branch, so this should also use `GetThreadCache()` to be consistent and ensure large allocations check the correct branch.
uint16_t active_bucket_count_ = 0;`active_bucket_count_` is read in the fast path `ThreadCache::Get()` without locks, but can be updated concurrently by `ThreadCacheRegistry::SetActiveBucketCount`. This constitutes a data race, which is Undefined Behavior in C++.\n\nConsider using `std::atomic<uint16_t>` with `std::memory_order_relaxed` here to avoid tearing and conform to the C++ memory model.
}Adding `|| IsTombstone(value)` here is recommended to mirror the POSIX implementation. If `PartitionTlsOnThreadExit` is somehow called again when the key is already tombstoned (e.g., during teardown after test execution), `value` could be `kTombstone`, bypassing the null check and crashing later on `PA_DCHECK`.
PartitionTlsSet(g_tls_key, nullptr);This should be set to `kTombstone` instead of `nullptr`, matching the POSIX implementation.\nSetting it to `nullptr` means that any calls to `GetTls()` during `~ThreadCache()` on component builds will see a null pointer and trigger `GetTlsSlowPath()`, which will improperly allocate a new `Tls` structure during thread destruction.\n\nAlso, `g_tls.state = TlsState::kTombstone;` should be moved here before the loop, so that non-component builds correctly return `nullptr` from `GetTls()` during destruction.
PartitionTlsSet(g_tls_key, reinterpret_cast<void*>(If you move `g_tls.state = TlsState::kTombstone;` and set `g_tls_key` to `kTombstone` before the loop, you can remove this block and the duplicate `PartitionTlsSet` call at the end of the function.
active_bucket_count_ = ThreadCacheRegistry::Instance().active_bucket_count_;Reading `active_bucket_count_` here outside of the registry lock causes a race condition. If `ThreadCacheRegistry::SetActiveBucketCount` runs between this read and `RegisterThreadCache`, this new thread cache will miss the update and retain the old `active_bucket_count_` indefinitely since it's not yet in the list.\n\nYou should set the thread cache's `active_bucket_count_` inside `RegisterThreadCache()` while holding the lock, rather than doing it here.
Related details
- Mikihito Matsuura
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |