From 03959a332bceef3817cf982eb0fe80334ccdb71f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Wed, 17 Mar 2021 12:11:43 +0200 Subject: [PATCH 1/3] reader_concurrency_semaphore: add permit-stats Which stores permit related stats. For now only total number of permits is maintained which is useful to determine whether the semaphore was used already or not. --- reader_concurrency_semaphore.cc | 6 ++++++ reader_concurrency_semaphore.hh | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/reader_concurrency_semaphore.cc b/reader_concurrency_semaphore.cc index 208349b8ad..4781f3a21f 100644 --- a/reader_concurrency_semaphore.cc +++ b/reader_concurrency_semaphore.cc @@ -168,6 +168,7 @@ struct reader_concurrency_semaphore::permit_list { using list_type = boost::intrusive::list>; list_type permits; + permit_stats stats; }; reader_permit::reader_permit(reader_concurrency_semaphore& semaphore, const schema* const schema, std::string_view op_name) @@ -605,6 +606,7 @@ future reader_concurrency_semaphore::do_wait_admi void reader_concurrency_semaphore::on_permit_created(reader_permit::impl& permit) { _permit_gate.enter(); _permit_list->permits.push_back(permit); + ++_permit_list->stats.total_permits; } void reader_concurrency_semaphore::on_permit_destroyed(reader_permit::impl& permit) noexcept { @@ -612,6 +614,10 @@ void reader_concurrency_semaphore::on_permit_destroyed(reader_permit::impl& perm _permit_gate.leave(); } +reader_concurrency_semaphore::permit_stats reader_concurrency_semaphore::get_permit_stats() const { + return _permit_list->stats; +} + reader_permit reader_concurrency_semaphore::make_permit(const schema* const schema, const char* const op_name) { return reader_permit(*this, schema, std::string_view(op_name)); } diff --git a/reader_concurrency_semaphore.hh b/reader_concurrency_semaphore.hh index b29681134e..510503a397 100644 --- a/reader_concurrency_semaphore.hh +++ b/reader_concurrency_semaphore.hh @@ -79,6 +79,10 @@ public: // Total number of reads rejected because the admission queue reached its max capacity uint64_t total_reads_shed_due_to_overload = 0; }; + struct permit_stats { + // Total number of permits created so far. + uint64_t total_permits = 0; + }; struct permit_list; @@ -282,6 +286,9 @@ public: return _stats; } + /// Return stats about the currently existing permits. + permit_stats get_permit_stats() const; + /// Make a permit /// /// The permit is associated with a schema, which is the schema of the table From 750b20fd852e045586cf2d54ca0dfb217cb08749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Mon, 12 Jul 2021 15:07:08 +0300 Subject: [PATCH 2/3] reader_concurrency_semaphore: allow destroying without stop() when not used yet To make it easier to construct objects with semaphore members. When the construction of such object fails, they can now just destroy their semaphore member as usual, without having to employ tricks to make sure its is stopped before. --- reader_concurrency_semaphore.cc | 4 ++++ test/boost/reader_concurrency_semaphore_test.cc | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/reader_concurrency_semaphore.cc b/reader_concurrency_semaphore.cc index 4781f3a21f..1e743bbe36 100644 --- a/reader_concurrency_semaphore.cc +++ b/reader_concurrency_semaphore.cc @@ -410,6 +410,10 @@ reader_concurrency_semaphore::reader_concurrency_semaphore(no_limits, sstring na std::move(name)) {} reader_concurrency_semaphore::~reader_concurrency_semaphore() { + if (!_permit_list->stats.total_permits) { + // We allow destroy without stop() when the semaphore wasn't used at all yet. + return; + } assert(_stopped); } diff --git a/test/boost/reader_concurrency_semaphore_test.cc b/test/boost/reader_concurrency_semaphore_test.cc index a706a8983f..b10d004f6b 100644 --- a/test/boost/reader_concurrency_semaphore_test.cc +++ b/test/boost/reader_concurrency_semaphore_test.cc @@ -554,6 +554,12 @@ SEASTAR_THREAD_TEST_CASE(reader_concurrency_semaphore_dump_reader_diganostics) { } SEASTAR_THREAD_TEST_CASE(test_reader_concurrency_semaphore_stop_waits_on_permits) { + BOOST_TEST_MESSAGE("unused"); + { + reader_concurrency_semaphore semaphore(reader_concurrency_semaphore::no_limits{}, get_name()); + // Checks for stop() should not be triggered. + } + BOOST_TEST_MESSAGE("0 permits"); { reader_concurrency_semaphore semaphore(reader_concurrency_semaphore::no_limits{}, get_name()); From f8004c652b6b17b0498a9a0faaf39195ef97cc8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Mon, 12 Jul 2021 15:18:25 +0300 Subject: [PATCH 3/3] reader_concurrency_semaphore: relax _stopped check when destroying a used semaphore Further relax the conditions under which we abort on destroying a unstopped semaphore. We already allow destroying completely unused semaphores, this patch further relaxes this to allow destroying formerly used but presently not used semaphores without stopping. We still call `on_internal_error_noexcept()` even if destroying the semaphore is safe, because without calling `stop()`, destroying the semaphore depends on luck, which we shouldn't rely on. --- reader_concurrency_semaphore.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/reader_concurrency_semaphore.cc b/reader_concurrency_semaphore.cc index 1e743bbe36..55c584fb1a 100644 --- a/reader_concurrency_semaphore.cc +++ b/reader_concurrency_semaphore.cc @@ -414,7 +414,13 @@ reader_concurrency_semaphore::~reader_concurrency_semaphore() { // We allow destroy without stop() when the semaphore wasn't used at all yet. return; } - assert(_stopped); + if (!_stopped) { + on_internal_error_noexcept(rcslog, format("~reader_concurrency_semaphore(): semaphore {} not stopped before destruction", _name)); + // With the below conditions, we can get away with the semaphore being + // unstopped. In this case don't force an abort. + assert(_inactive_reads.empty() && !_close_readers_gate.get_count() && !_permit_gate.get_count()); + broken(); + } } reader_concurrency_semaphore::inactive_read_handle reader_concurrency_semaphore::register_inactive_read(flat_mutation_reader reader) noexcept {