From patchwork Mon May 4 13:36:34 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1282648 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=gcc.gnu.org Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=lrlL4S11; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49G3mq3kq3z9sSc for ; Mon, 4 May 2020 23:36:58 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9C9793887019; Mon, 4 May 2020 13:36:51 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9C9793887019 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1588599411; bh=RUJX+NYXjGiVenx/PwINWerYrqpnwk6h0kpDaJDcK+s=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=lrlL4S11PGVatOQcC+f817HojC/rt6ku7/aOdAHo891kkWLDwNmjQ2NqmP2DVWGsT TwEBVIP+Gbvq37vw1Ljj2JQMwu3yr/qYjC3tc4wiL/BwKQg/Mi/EQLkICzz8LuXwoH B3zuIFl2/wCRB32RLHvPNd54SpCBliJhRnOa8SQA= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id E6AA83887013 for ; Mon, 4 May 2020 13:36:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E6AA83887013 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-223-aP-5f9YOPVydRH9vYzxpuA-1; Mon, 04 May 2020 09:36:36 -0400 X-MC-Unique: aP-5f9YOPVydRH9vYzxpuA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 641EC462; Mon, 4 May 2020 13:36:35 +0000 (UTC) Received: from localhost (unknown [10.33.36.241]) by smtp.corp.redhat.com (Postfix) with ESMTP id DD1EB600F5; Mon, 4 May 2020 13:36:34 +0000 (UTC) Date: Mon, 4 May 2020 14:36:34 +0100 To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Make pmr::synchronized_pool_resource work without libpthread (PR 94936) Message-ID: <20200504133634.GA730513@redhat.com> MIME-Version: 1.0 X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-27.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" I implicitly assumed that programs using pmr::synchronized_pool_resource would also be using multiple threads, and so the weak symbols in gthr-posix.h would be resolved by linking to libpthread. If that isn't true then it crashes when trying to use pthread_key_create. This commit makes the pool resource check __gthread_active_p() before using thread-specific data, and just use a single set of memory pools when there's only a single thread. PR libstdc++/94936 * src/c++17/memory_resource.cc (synchronized_pool_resource::_TPools): Add comment about single-threaded behaviour. (synchronized_pool_resource::_TPools::move_nonempty_chunks()): Hoist class member access out of loop. (synchronized_pool_resource::synchronized_pool_resource()) (synchronized_pool_resource::~synchronized_pool_resource()) (synchronized_pool_resource::release()): Check __gthread_active_p before creating and/or deleting the thread-specific data key. (synchronized_pool_resource::_M_thread_specific_pools()): Adjust assertions. (synchronized_pool_resource::do_allocate(size_t, size_t)): Add fast path for single-threaded case. (synchronized_pool_resource::do_deallocate(void*, size_t, size_t)): Likewise. Return if unable to find a pool that owns the allocation. * testsuite/20_util/synchronized_pool_resource/allocate_single.cc: New test. * testsuite/20_util/synchronized_pool_resource/cons_single.cc: New test. * testsuite/20_util/synchronized_pool_resource/release_single.cc: New test. Tested powerpc64le-linux, committed to master. I'll backport this, but after 10.1 is released. commit ec40967f1323069da3a5a45286f71fa4f80926df Author: Jonathan Wakely Date: Mon May 4 13:34:23 2020 +0100 libstdc++: Make pmr::synchronized_pool_resource work without libpthread (PR 94936) I implicitly assumed that programs using pmr::synchronized_pool_resource would also be using multiple threads, and so the weak symbols in gthr-posix.h would be resolved by linking to libpthread. If that isn't true then it crashes when trying to use pthread_key_create. This commit makes the pool resource check __gthread_active_p() before using thread-specific data, and just use a single set of memory pools when there's only a single thread. PR libstdc++/94936 * src/c++17/memory_resource.cc (synchronized_pool_resource::_TPools): Add comment about single-threaded behaviour. (synchronized_pool_resource::_TPools::move_nonempty_chunks()): Hoist class member access out of loop. (synchronized_pool_resource::synchronized_pool_resource()) (synchronized_pool_resource::~synchronized_pool_resource()) (synchronized_pool_resource::release()): Check __gthread_active_p before creating and/or deleting the thread-specific data key. (synchronized_pool_resource::_M_thread_specific_pools()): Adjust assertions. (synchronized_pool_resource::do_allocate(size_t, size_t)): Add fast path for single-threaded case. (synchronized_pool_resource::do_deallocate(void*, size_t, size_t)): Likewise. Return if unable to find a pool that owns the allocation. * testsuite/20_util/synchronized_pool_resource/allocate_single.cc: New test. * testsuite/20_util/synchronized_pool_resource/cons_single.cc: New test. * testsuite/20_util/synchronized_pool_resource/release_single.cc: New test. diff --git a/libstdc++-v3/src/c++17/memory_resource.cc b/libstdc++-v3/src/c++17/memory_resource.cc index 56a87844da0..1acab19e306 100644 --- a/libstdc++-v3/src/c++17/memory_resource.cc +++ b/libstdc++-v3/src/c++17/memory_resource.cc @@ -1043,7 +1043,8 @@ namespace pmr * exposition as _M_tpools[_M_key]). * The first element, _M_tpools[0], contains "orphaned chunks" which were * allocated by a thread which has since exited, and so there is no - * _M_tpools[_M_key] for that thread. + * _M_tpools[_M_key] for that thread. Orphaned chunks are never reused, + * they're only held in _M_tpools[0] so they can be deallocated. * A thread can access its own thread-specific set of pools via _M_key * while holding a shared lock on _M_mx. Accessing _M_impl._M_unpooled * or _M_tpools[0] or any other thread's _M_tpools[_M_key] requires an @@ -1052,6 +1053,10 @@ namespace pmr * any dereference of that pointer requires an exclusive lock. * The _M_impl._M_opts and _M_impl._M_npools members are immutable, * and can safely be accessed concurrently. + * + * In a single-threaded program (i.e. __gthread_active_p() == false) + * the pool resource only needs one set of pools and never has orphaned + * chunks, so just uses _M_tpools[0] directly, and _M_tpools->next is null. */ extern "C" { @@ -1092,14 +1097,16 @@ namespace pmr void move_nonempty_chunks() { __glibcxx_assert(pools); + __glibcxx_assert(__gthread_active_p()); if (!pools) return; - memory_resource* r = owner.upstream_resource(); + memory_resource* const r = owner.upstream_resource(); + auto* const shared = owner._M_tpools->pools; // move all non-empty chunks to the shared _TPools for (int i = 0; i < owner._M_impl._M_npools; ++i) for (auto& c : pools[i]._M_chunks) if (!c.empty()) - owner._M_tpools->pools[i]._M_chunks.insert(std::move(c), r); + shared[i]._M_chunks.insert(std::move(c), r); } synchronized_pool_resource& owner; @@ -1133,8 +1140,9 @@ namespace pmr memory_resource* upstream) : _M_impl(opts, upstream) { - if (int err = __gthread_key_create(&_M_key, destroy_TPools)) - __throw_system_error(err); + if (__gthread_active_p()) + if (int err = __gthread_key_create(&_M_key, destroy_TPools)) + __throw_system_error(err); exclusive_lock l(_M_mx); _M_tpools = _M_alloc_shared_tpools(l); } @@ -1143,7 +1151,8 @@ namespace pmr synchronized_pool_resource::~synchronized_pool_resource() { release(); - __gthread_key_delete(_M_key); // does not run destroy_TPools + if (__gthread_active_p()) + __gthread_key_delete(_M_key); // does not run destroy_TPools } void @@ -1152,8 +1161,11 @@ namespace pmr exclusive_lock l(_M_mx); if (_M_tpools) { - __gthread_key_delete(_M_key); // does not run destroy_TPools - __gthread_key_create(&_M_key, destroy_TPools); + if (__gthread_active_p()) + { + __gthread_key_delete(_M_key); // does not run destroy_TPools + __gthread_key_create(&_M_key, destroy_TPools); + } polymorphic_allocator<_TPools> a(upstream_resource()); // destroy+deallocate each _TPools do @@ -1175,10 +1187,11 @@ namespace pmr synchronized_pool_resource::_M_thread_specific_pools() noexcept { __pool_resource::_Pool* pools = nullptr; + __glibcxx_assert(__gthread_active_p()); if (auto tp = static_cast<_TPools*>(__gthread_getspecific(_M_key))) { - pools = tp->pools; - __glibcxx_assert(tp->pools); + pools = tp->pools; + // __glibcxx_assert(tp->pools); } return pools; } @@ -1189,24 +1202,34 @@ namespace pmr do_allocate(size_t bytes, size_t alignment) { const auto block_size = std::max(bytes, alignment); - if (block_size <= _M_impl._M_opts.largest_required_pool_block) + const pool_options opts = _M_impl._M_opts; + if (block_size <= opts.largest_required_pool_block) { const ptrdiff_t index = pool_index(block_size, _M_impl._M_npools); - memory_resource* const r = upstream_resource(); - const pool_options opts = _M_impl._M_opts; - { - // Try to allocate from the thread-specific pool - shared_lock l(_M_mx); - if (auto pools = _M_thread_specific_pools()) // [[likely]] - { - // Need exclusive lock to replenish so use try_allocate: - if (void* p = pools[index].try_allocate()) - return p; - // Need to take exclusive lock and replenish pool. - } - // Need to allocate or replenish thread-specific pools using - // upstream resource, so need to hold exclusive lock. - } + if (__gthread_active_p()) + { + // Try to allocate from the thread-specific pool. + shared_lock l(_M_mx); + if (auto pools = _M_thread_specific_pools()) // [[likely]] + { + // Need exclusive lock to replenish so use try_allocate: + if (void* p = pools[index].try_allocate()) + return p; + // Need to take exclusive lock and replenish pool. + } + // Need to allocate or replenish thread-specific pools using + // upstream resource, so need to hold exclusive lock. + } + else // single-threaded + { + if (!_M_tpools) // [[unlikely]] + { + exclusive_lock dummy(_M_mx); + _M_tpools = _M_alloc_shared_tpools(dummy); + } + return _M_tpools->pools[index].allocate(upstream_resource(), opts); + } + // N.B. Another thread could call release() now lock is not held. exclusive_lock excl(_M_mx); if (!_M_tpools) // [[unlikely]] @@ -1214,7 +1237,7 @@ namespace pmr auto pools = _M_thread_specific_pools(); if (!pools) pools = _M_alloc_tpools(excl)->pools; - return pools[index].allocate(r, opts); + return pools[index].allocate(upstream_resource(), opts); } exclusive_lock l(_M_mx); return _M_impl.allocate(bytes, alignment); // unpooled allocation @@ -1230,30 +1253,45 @@ namespace pmr { const ptrdiff_t index = pool_index(block_size, _M_impl._M_npools); __glibcxx_assert(index != -1); - { - shared_lock l(_M_mx); - auto pools = _M_thread_specific_pools(); - if (pools) - { - // No need to lock here, no other thread is accessing this pool. - if (pools[index].deallocate(upstream_resource(), p)) - return; - } - // Block might have come from a different thread's pool, - // take exclusive lock and check every pool. - } + if (__gthread_active_p()) + { + shared_lock l(_M_mx); + if (auto pools = _M_thread_specific_pools()) + { + // No need to lock here, no other thread is accessing this pool. + if (pools[index].deallocate(upstream_resource(), p)) + return; + } + // Block might have come from a different thread's pool, + // take exclusive lock and check every pool. + } + else // single-threaded + { + __glibcxx_assert(_M_tpools != nullptr); + if (_M_tpools) // [[likely]] + _M_tpools->pools[index].deallocate(upstream_resource(), p); + return; + } + // TODO store {p, bytes, alignment} somewhere and defer returning // the block to the correct thread-specific pool until we next // take the exclusive lock. + exclusive_lock excl(_M_mx); + auto my_pools = _M_thread_specific_pools(); for (_TPools* t = _M_tpools; t != nullptr; t = t->next) { - if (t->pools) // [[likely]] - { - if (t->pools[index].deallocate(upstream_resource(), p)) - return; - } + if (t->pools != my_pools) + if (t->pools) // [[likely]] + { + if (t->pools[index].deallocate(upstream_resource(), p)) + return; + } } + // Not necessarily an error to reach here, release() could have been + // called on another thread between releasing the shared lock and + // acquiring the exclusive lock. + return; } exclusive_lock l(_M_mx); _M_impl.deallocate(p, bytes, alignment); @@ -1265,6 +1303,7 @@ namespace pmr -> _TPools* { __glibcxx_assert(_M_tpools != nullptr); + __glibcxx_assert(__gthread_active_p()); // dump_list(_M_tpools); polymorphic_allocator<_TPools> a(upstream_resource()); _TPools* p = a.allocate(1); diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc new file mode 100644 index 00000000000..d5b7b162146 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/allocate_single.cc @@ -0,0 +1,24 @@ +// Copyright (C) 2018-2020 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-do run } +// { dg-options "-std=gnu++17" } +// { dg-require-effective-target c++17 } +// { dg-require-gthreads "" } + +// This runs the same tests as allocate.cc but without -pthread +#include "./allocate.cc" diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc new file mode 100644 index 00000000000..060cd7628e5 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/cons_single.cc @@ -0,0 +1,24 @@ +// Copyright (C) 2018-2020 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-do run } +// { dg-options "-std=gnu++17" } +// { dg-require-effective-target c++17 } +// { dg-require-gthreads "" } + +// This runs the same tests as cons.cc but without -pthread +#include "./cons.cc" diff --git a/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc new file mode 100644 index 00000000000..32e80c1be3f --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/synchronized_pool_resource/release_single.cc @@ -0,0 +1,24 @@ +// Copyright (C) 2018-2020 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-do run } +// { dg-options "-std=gnu++17" } +// { dg-require-effective-target c++17 } +// { dg-require-gthreads "" } + +// This runs the same tests as release.cc but without -pthread +#include "./release.cc"