This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new fd08fdd268 Use `this_ethread()` for evacuation block alloc (#11623)
fd08fdd268 is described below
commit fd08fdd268a83c5236599cd914a3f1953bd42154
Author: JosiahWI <[email protected]>
AuthorDate: Tue Jul 30 14:48:47 2024 -0500
Use `this_ethread()` for evacuation block alloc (#11623)
* Use `this_ethread()` for evacuation block alloc
I would like to extract a new class to handle the responsibility of document
evacuation, but that is hard because the evacuation logic uses the thread
holding the `StripeSM` mutex to do the `EvacuationBlock` allocation. I think
this is a hacky way to get the current thread, so I want to switch the calls
to `this_ethread()` which doesn't depend on the mutex's internal data and
describes precisely what it does. I'm PRing this change all by itself
because
from the perspective of the `StripeSM` API it is a behavior change. I don't
think there can be too many debug asserts for this change so I've added them
to every call into the evacuation logic, although if any of these asserts
were
violated we'd probably already have seen horrendous damage to the
unprotected
doubly-linked `evacuate` list and more.
* Implement suggestion by Chris McFarlen
* Call `this_ethread()` directly in the `THREAD_ALLOC` invocation.
This is how it works everywhere else. Fewer parameters, more consistency.
Yay!!
---
src/iocore/cache/CacheDir.cc | 8 ++++----
src/iocore/cache/CacheRead.cc | 5 +++++
src/iocore/cache/P_CacheVol.h | 38 +++++++++++++++++++++++++++++---------
src/iocore/cache/StripeSM.cc | 15 +++++++++------
4 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/src/iocore/cache/CacheDir.cc b/src/iocore/cache/CacheDir.cc
index c50b872ddd..a68e8767d8 100644
--- a/src/iocore/cache/CacheDir.cc
+++ b/src/iocore/cache/CacheDir.cc
@@ -833,7 +833,7 @@ dir_lookaside_insert(EvacuationBlock *eblock, StripeSM
*stripe, Dir *to)
(int)dir_phase(to));
ink_assert(stripe->mutex->thread_holding == this_ethread());
int i = key->slice32(3) % LOOKASIDE_SIZE;
- EvacuationBlock *b =
new_EvacuationBlock(stripe->mutex->thread_holding);
+ EvacuationBlock *b = new_EvacuationBlock();
b->evac_frags.key = *key;
b->evac_frags.earliest_key = *key;
b->earliest_evacuator = eblock->earliest_evacuator;
@@ -858,7 +858,7 @@ dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe)
int64_t o = dir_offset(&b->dir), n = dir_offset(&b->new_dir);
stripe->ram_cache->fixup(key, static_cast<uint64_t>(o),
static_cast<uint64_t>(n));
stripe->lookaside[i].remove(b);
- free_EvacuationBlock(b, stripe->mutex->thread_holding);
+ free_EvacuationBlock(b);
return res;
}
b = b->link.next;
@@ -880,7 +880,7 @@ dir_lookaside_cleanup(StripeSM *stripe)
b->evac_frags.earliest_key.slice32(1));
i.remove(b);
free_CacheEvacuateDocVC(b->earliest_evacuator);
- free_EvacuationBlock(b, stripe->mutex->thread_holding);
+ free_EvacuationBlock(b);
b = nb;
goto Lagain;
}
@@ -901,7 +901,7 @@ dir_lookaside_remove(const CacheKey *key, StripeSM *stripe)
DDbg(dbg_ctl_dir_lookaside, "remove %X %X offset %" PRId64 " phase %d",
key->slice32(0), key->slice32(1),
dir_offset(&b->new_dir), dir_phase(&b->new_dir));
stripe->lookaside[i].remove(b);
- free_EvacuationBlock(b, stripe->mutex->thread_holding);
+ free_EvacuationBlock(b);
return;
}
b = b->link.next;
diff --git a/src/iocore/cache/CacheRead.cc b/src/iocore/cache/CacheRead.cc
index 5fdbf5e125..4cd1a34a97 100644
--- a/src/iocore/cache/CacheRead.cc
+++ b/src/iocore/cache/CacheRead.cc
@@ -24,6 +24,10 @@
#include "P_Cache.h"
#include "P_CacheDoc.h"
+#ifdef DEBUG
+#include "iocore/eventsystem/EThread.h"
+#endif
+
namespace
{
@@ -559,6 +563,7 @@ CacheVC::openReadClose(int event, Event * /* e ATS_UNUSED
*/)
VC_SCHED_LOCK_RETRY();
}
if (f.hit_evacuate && dir_valid(stripe, &first_dir) && closed > 0) {
+ ink_assert(stripe->mutex->thread_holding == this_ethread());
if (f.single_fragment) {
stripe->force_evacuate_head(&first_dir, dir_pinned(&first_dir));
} else if (dir_valid(stripe, &earliest_dir)) {
diff --git a/src/iocore/cache/P_CacheVol.h b/src/iocore/cache/P_CacheVol.h
index a448b41f84..2098eafa99 100644
--- a/src/iocore/cache/P_CacheVol.h
+++ b/src/iocore/cache/P_CacheVol.h
@@ -196,11 +196,31 @@ public:
int evacuateWrite(CacheEvacuateDocVC *evacuator, int event, Event *e);
int evacuateDocReadDone(int event, Event *e);
- int evac_range(off_t start, off_t end, int evac_phase);
- void periodic_scan();
- void scan_for_pinned_documents();
- void evacuate_cleanup_blocks(int i);
- void evacuate_cleanup();
+ int evac_range(off_t start, off_t end, int evac_phase);
+ /**
+ *
+ * The caller must hold the mutex.
+ */
+ void periodic_scan();
+ /**
+ *
+ * The caller must hold the mutex.
+ */
+ void scan_for_pinned_documents();
+ /**
+ *
+ * The caller must hold the mutex.
+ */
+ void evacuate_cleanup_blocks(int i);
+ /**
+ *
+ * The caller must hold the mutex.
+ */
+ void evacuate_cleanup();
+ /**
+ *
+ * The caller must hold the mutex.
+ */
EvacuationBlock *force_evacuate_head(Dir const *dir, int pinned);
int within_hit_evacuate_window(Dir const *dir) const;
@@ -310,9 +330,9 @@ StripeSM::cancel_trigger()
}
inline EvacuationBlock *
-new_EvacuationBlock(EThread *t)
+new_EvacuationBlock()
{
- EvacuationBlock *b = THREAD_ALLOC(evacuationBlockAllocator, t);
+ EvacuationBlock *b = THREAD_ALLOC(evacuationBlockAllocator,
this_ethread());
b->init = 0;
b->readers = 0;
b->earliest_evacuator = nullptr;
@@ -321,7 +341,7 @@ new_EvacuationBlock(EThread *t)
}
inline void
-free_EvacuationBlock(EvacuationBlock *b, EThread *t)
+free_EvacuationBlock(EvacuationBlock *b)
{
EvacuationKey *e = b->evac_frags.link.next;
while (e) {
@@ -329,7 +349,7 @@ free_EvacuationBlock(EvacuationBlock *b, EThread *t)
evacuationKeyAllocator.free(e);
e = n;
}
- THREAD_FREE(b, evacuationBlockAllocator, t);
+ THREAD_FREE(b, evacuationBlockAllocator, this_ethread());
}
inline OpenDirEntry *
diff --git a/src/iocore/cache/StripeSM.cc b/src/iocore/cache/StripeSM.cc
index 2368c5283a..2e1fc0769b 100644
--- a/src/iocore/cache/StripeSM.cc
+++ b/src/iocore/cache/StripeSM.cc
@@ -133,8 +133,7 @@ StripeSM::begin_read(CacheVC *cont) const
}
// we don't actually need to preserve this block as it is already in
// memory, but this is easier, and evacuations are rare
- EThread *t = cont->mutex->thread_holding;
- b = new_EvacuationBlock(t);
+ b = new_EvacuationBlock();
b->readers = 1;
b->dir = cont->earliest_dir;
b->evac_frags.key = cont->earliest_key;
@@ -161,7 +160,7 @@ StripeSM::close_read(CacheVC *cont) const
}
if (b->readers && !--b->readers) {
evacuate[i].remove(b);
- free_EvacuationBlock(b, t);
+ free_EvacuationBlock(b);
break;
}
b = next;
@@ -663,6 +662,7 @@ StripeSM::handle_recover_write_dir(int /* event ATS_UNUSED
*/, void * /* data AT
init_info = nullptr;
set_io_not_in_progress();
scan_pos = header->write_pos;
+ ink_assert(this->mutex->thread_holding = this_ethread());
periodic_scan();
SET_HANDLER(&StripeSM::dir_init_done);
return dir_init_done(EVENT_IMMEDIATE, nullptr);
@@ -721,7 +721,7 @@ StripeSM::evacuate_cleanup_blocks(int i)
DDbg(dbg_ctl_cache_evac, "evacuate cleanup free %X offset %d",
(int)b->evac_frags.key.slice32(0), (int)dir_offset(&b->dir));
b = b->link.next;
evacuate[i].remove(x);
- free_EvacuationBlock(x, mutex->thread_holding);
+ free_EvacuationBlock(x);
continue;
}
b = b->link.next;
@@ -778,7 +778,7 @@ StripeSM::force_evacuate_head(Dir const *evac_dir, int
pinned)
}
if (!b) {
- b = new_EvacuationBlock(mutex->thread_holding);
+ b = new_EvacuationBlock();
b->dir = *evac_dir;
DDbg(dbg_ctl_cache_evac, "force: %d, %d", (int)dir_offset(evac_dir),
(int)dir_phase(evac_dir));
evacuate[bucket].push(b);
@@ -904,6 +904,7 @@ StripeSM::aggWriteDone(int event, Event *e)
header->last_write_pos);
ink_assert(header->write_pos == header->agg_pos);
if (header->write_pos + EVACUATION_SIZE > scan_pos) {
+ ink_assert(this->mutex->thread_holding == this_ethread());
periodic_scan();
}
this->_write_buffer.reset_buffer_pos();
@@ -1260,6 +1261,7 @@ StripeSM::agg_wrap()
Metrics::Counter::increment(stripe->cache_vol->vol_rsb.directory_wrap);
Note("Cache volume %d on disk '%s' wraps around",
stripe->cache_vol->vol_number, stripe->hash_text.get());
}
+ ink_assert(this->mutex->thread_holding = this_ethread());
periodic_scan();
}
@@ -1393,6 +1395,7 @@ StripeSM::evacuateDocReadDone(int event, Event *e)
// Cache::open_write).
if (!dir_head(&b->dir) || !dir_compare_tag(&b->dir, &doc->first_key)) {
next_CacheKey(&next_key, &doc->key);
+ ink_assert(this->mutex->thread_holding == this_ethread());
evacuate_fragments(&next_key, &doc_evacuator->earliest_key, !b->readers,
this);
}
return evacuateWrite(doc_evacuator, event, e);
@@ -1415,7 +1418,7 @@ evacuate_fragments(CacheKey *key, CacheKey *earliest_key,
int force, StripeSM *s
}
EvacuationBlock *b = evacuation_block_exists(&dir, stripe);
if (!b) {
- b =
new_EvacuationBlock(stripe->mutex->thread_holding);
+ b = new_EvacuationBlock();
b->dir = dir;
b->evac_frags.key = *key;
b->evac_frags.earliest_key = *earliest_key;
