From df47e3a9a462fcad6118e53d805d01df9de236fe Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 26 Aug 2026 21:27:01 +0900 Subject: [PATCH] Report SS2PL lock acquisition failures to the caller `TxExecutor::read()` and `TxExecutor::update()` in `cc/ss2pl/transaction.cc` set `status_ = aborted` when a lock cannot be taken, and then return `Status::OK` anyway. A caller that looks at the return value sees success, so `read()` leaves the caller's `TupleBody*` pointing at an unrelated `read_set_` entry (or at nothing, when the read set is empty) and the caller dereferences it. `tpcc_ss2pl.exe` dies from this deterministically. TPC-C Payment updates the Warehouse row of every transaction, which is the most contended row in the benchmark, so with two or more threads a write lock conflict happens within seconds. Measured on an Intel Xeon Platinum 8468 (48 cores), Release, g++ 11.4.0, default `CCBENCH_SS2PL_DLR=1`, `-extime=3`, three runs per thread count: | `-thread_num` | before | after | |---|---|---| | 1 | 3/3 exit 0 | 3/3 exit 0 | | 2 | 3/3 SIGSEGV | 3/3 exit 0 | | 4 | 3/3 SIGSEGV | 3/3 exit 0 | | 8 | 3/3 SIGSEGV | 3/3 exit 0 | | 48 | 3/3 SIGSEGV | 3/3 exit 0 | Both columns come from the same job on the same host, so the failure and the fix are measured against each other rather than against a remembered result. PR #121 added exactly this guard, but only inside `#if defined(DLR0)`, because that change had promised not to touch the DLR1 code at all. The defect is in DLR1 as well, and DLR1 is the default, so the guards become unconditional here. - `cc/ss2pl/transaction.cc`: the two `#if defined(DLR0)` / `#endif` pairs around the `status_ == aborted` guards in `read()` and `update()` are removed. Nothing inside them changes, and neither does any lock acquisition branch. `scan()`, `read_lock()` and `write_lock()` already returned `Status::ERROR_LOCK_FAILED` unconditionally, so this makes the five `Status`-returning operations agree with each other. - `include/bomb_pessimistic.hh`: `run_update_material_cost_master()` checked neither the return value nor `status_` before dereferencing `body`. This is the pre-existing problem that the #121 commit message listed as deliberately unaddressed; it has to be handled now, because the new return value stops `read()` from writing `*body` at all. It now follows the form that `docs/coding-conventions_ja.md` prescribes and that `get_material_cost()` fifteen lines below already uses. This header is included only by `cc/ss2pl/bomb_ss2pl.cc`, so no other protocol is affected. - `include/tpcc/tpcc_tx_delivery.hh`: in `update_order_line_and_get_ol_total()`, `if (status != Status::OK) ERR;` ran before the `status_ == aborted` check, so a reported lock failure would call `ERR` and `exit(1)`. The two lines are swapped, which matches `update_customer_balance()` in the same file and the `scan()` handling fifteen lines above. The only state whose behaviour changes is `status != Status::OK && status_ == aborted`, which used to end the process and is now an ordinary abort and retry. - `docs/coding-conventions_{ja,en}.md`: the description of `tx.read`'s result listed only `OK` and `WARN_NOT_FOUND`. Protocol-specific errors are now mentioned, and the rule is stated as "reject every non-OK value" rather than as a test against one constant. The delivery fix is not optional. Building only the `cc/ss2pl/transaction.cc` half of this commit and running `tpcc_ss2pl.exe -thread_num=48 -extime=3` gives exit code 1 with `ERR` from `tpcc_tx_delivery.hh` in all three runs: the SIGSEGV is replaced by a process abort rather than removed. With the caller adaptations, the same command exits 0. Callers audited by following the reference graph rather than by grepping. `cc/ss2pl/CMakeLists.txt` builds `ycsb`, `bomb` and `tpcc`; `cc/ss2pl/test/` is not added by any `add_subdirectory` and is not built. Those three entrypoints reach 34 lexical `read()` / `update()` call sites, 31 of them reachable (`include/bomb_pessimistic.hh` lines 904, 913 and 930 are in helpers that nothing calls). Every TPC-C site already tested `stat != Status::OK`, so they now decline safely. Every YCSB site tests `tx.status_` immediately after the call, so its retry behaviour is unchanged. Only the BoMB site above tested neither. No regression, same host and job, 48 threads, `-extime=5`, three runs each, median reported: | workload | before | after | |---|---|---| | YCSB, 1M records, zipf 0.9 | 682,581 tps / 0.8731 | 680,706 tps / 0.8743 | | YCSB, 1M records, uniform | 7,621,903 tps / 0.0031 | 7,618,854 tps / 0.0032 | | YCSB, 100 records, zipf 0.9 | 439,287 tps / 0.2290 | 440,740 tps / 0.2378 | | BoMB, 8 threads | 19,654 tps / 0.9887 | 20,175 tps / 0.9794 | | BoMB, 48 threads | 23,211 tps / 0.9955 | 22,394 tps / 0.9922 | DLR0 keeps the contract from #121. Its lock and timeout implementation is not touched: `cc/ss2pl/include/dlr0_timeout.hh`, `cc/ss2pl/util.cc`, `cc/ss2pl/CMakeLists.txt`, `cmake/Options.cmake` and `include/rwlock.hh` have no diff. What changes for DLR0 is that its callers now handle the `ERROR_LOCK_FAILED` it already returned. Measured with `CCBENCH_SS2PL_DLR=0`: the cycle-forming configuration (YCSB-A, 1,000,000 records, zipf skew 0.9, 48 threads) finishes in 6 s at the default timeout, the same configuration with `-ss2pl_dlr0_timeout_us=3600000000` is still running when a 120 s external watchdog kills it, and `tpcc_ss2pl.exe` exits 0 at 2, 8 and 48 threads. The reordered lines live in a header shared by every protocol that builds TPC-C, so `tpcc_.exe -thread_num=48 -extime=3` was run before and after, one run each. All eight other protocols end the same way in both: `tictoc`, `cicada` and `mocc` exit 0; `silo` and `mvto` are still running when a 300 s watchdog kills them; `si` and `ermia` exit 1 through an `ERR` in their own `garbage_collection.cc`. `oze` aborted before and exited 0 after, but from `std::out_of_range` thrown by `std::map::at` inside its own `TxExecutor::write_validation()`, which this change does not touch; with one run each that difference is not attributable here. In other words, no other protocol was observed reaching the state whose handling changed. Release builds of every protocol with `-Wall -Wextra -Werror` pass for `CCBENCH_SS2PL_DLR` 0 and 1 with both g++ 11.4.0 and g++ 12.3.0. The `clang-format --dry-run --Werror` check from `.github/workflows/format.yml` passes with clang-format 14. Not verified: GCC 13. The CI image uses it; it was not available on the test machine, so the `-Werror` result under the CI compiler is unconfirmed. Deliberately not addressed here: - `include/ycsb.hh` discards the `read()` return value. A lock failure is still caught by the `status_` check that follows, but a `WARN_NOT_FOUND` would not be. YCSB creates every key it reads, so this is latent. - `include/bomb.hh`, `include/sbomb_deterministic.hh` and `include/dbomb_deterministic.hh` have the same unchecked read as the BoMB header fixed here. They belong to the other protocols and none of those protocols' `read()` currently reaches this state in the same way. - Other protocols have their own instances of setting `status_ = aborted` while returning `Status::OK` (cicada, ermia, mocc, oze, si, silo and tictoc, mostly in `scan()` and its callback). One branch, one purpose: they are reported, not changed. - `tpcc_ss2pl.exe` reports "insert order failed" heavily even single-threaded, on the parent commit as well as here, and the count moves with this change (15,046 before, 13,005 after, one thread, 3 s). The cause was not established, so no claim is made about it either way. This change was written with AI assistance. --- cc/ss2pl/transaction.cc | 4 ---- docs/coding-conventions_en.md | 5 ++++- docs/coding-conventions_ja.md | 2 +- include/bomb_pessimistic.hh | 6 ++++-- include/tpcc/tpcc_tx_delivery.hh | 2 +- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/cc/ss2pl/transaction.cc b/cc/ss2pl/transaction.cc index 8936b2f1..b1437957 100644 --- a/cc/ss2pl/transaction.cc +++ b/cc/ss2pl/transaction.cc @@ -158,11 +158,9 @@ Status TxExecutor::read(Storage s, std::string_view key, TupleBody** body) { if (tuple == nullptr) return Status::WARN_NOT_FOUND; read_internal(s, key, tuple); -#if defined(DLR0) if (this->status_ == TransactionStatus::aborted) { return Status::ERROR_LOCK_FAILED; } -#endif *body = &(read_set_.back().body_); FINISH_READ: @@ -342,11 +340,9 @@ Status TxExecutor::update(Storage s, std::string_view key, TupleBody&& body) { #if ADD_ANALYSIS result_->local_write_latency_ += rdtscp() - start; #endif // ADD_ANALYSIS -#if defined(DLR0) if (this->status_ == TransactionStatus::aborted) { return Status::ERROR_LOCK_FAILED; } -#endif return Status::OK; } diff --git a/docs/coding-conventions_en.md b/docs/coding-conventions_en.md index 61ca8701..d6378ab6 100644 --- a/docs/coding-conventions_en.md +++ b/docs/coding-conventions_en.md @@ -69,7 +69,10 @@ Starts thin. Add as you learn. ### `tx.read` returns Status — *check it* `tx.read` returns `Status::OK` on hit and `Status::WARN_NOT_FOUND` on miss. -On miss, `*body` is **left unchanged** (it does not get nullified). Checking +Some protocols return other errors as well (`Status::ERROR_LOCK_FAILED` for +SS2PL and MOCC, `Status::ERROR_PREEMPTIVE_ABORT` for TicToc). **Reject every +non-`Status::OK` value** — do not test for one specific value. On any non-OK +result, `*body` is **left unchanged** (it does not get nullified). Checking only `tx.status_ == TransactionStatus::aborted` is not enough — a stale `body` pointer from a previous read will silently get dereferenced, which manifests as a `HeapObject::cast_to` assertion under Debug+ASan and as diff --git a/docs/coding-conventions_ja.md b/docs/coding-conventions_ja.md index 9ed3190e..5f94f5eb 100644 --- a/docs/coding-conventions_ja.md +++ b/docs/coding-conventions_ja.md @@ -62,7 +62,7 @@ ### `tx.read` の戻り値 Status は **必ず check する** -`tx.read` はヒット時に `Status::OK`、ミス時に `Status::WARN_NOT_FOUND` を返す。ミス時には `*body` は **そのまま** (= nullify されない) なので、`tx.status_ == TransactionStatus::aborted` だけチェックしても不十分 — 前回の read で得た古い `body` ポインタがそのまま deref されてしまい、Debug+ASan では `HeapObject::cast_to` の assertion で死に、Release では garbage data を読む。必ず次の形で書く: +`tx.read` はヒット時に `Status::OK`、ミス時に `Status::WARN_NOT_FOUND` を返す。protocol によっては、これ以外の error も返る (SS2PL と MOCC の `Status::ERROR_LOCK_FAILED`、TicToc の `Status::ERROR_PREEMPTIVE_ABORT` など)。**`Status::OK` 以外は全部拒否すること** — 特定の値だけを見てはいけない。`Status::OK` 以外のとき `*body` は **そのまま** (= nullify されない) なので、`tx.status_ == TransactionStatus::aborted` だけチェックしても不十分 — 前回の read で得た古い `body` ポインタがそのまま deref されてしまい、Debug+ASan では `HeapObject::cast_to` の assertion で死に、Release では garbage data を読む。必ず次の形で書く: ```cpp Status stat = tx.read(s, key, &body); diff --git a/include/bomb_pessimistic.hh b/include/bomb_pessimistic.hh index 9e49e006..6e8f4f64 100644 --- a/include/bomb_pessimistic.hh +++ b/include/bomb_pessimistic.hh @@ -695,8 +695,10 @@ public: for (auto& m_id : query.args.i_id_set) { SimpleKey<8> key; MaterialCostMaster::CreateKey(query.args.f_id, m_id, key.ptr()); - TupleBody* body; - tx.read(Storage::MaterialCostMaster, key.view(), &body); + TupleBody* body = nullptr; + Status stat = tx.read(Storage::MaterialCostMaster, key.view(), &body); + if (tx.status_ == TransactionStatus::aborted) return; + if (stat != Status::OK) return; MaterialCostMaster& old = body->get_value().cast_to(); HeapObject obj; obj.template allocate(); diff --git a/include/tpcc/tpcc_tx_delivery.hh b/include/tpcc/tpcc_tx_delivery.hh index 28ed6e98..bee855b6 100644 --- a/include/tpcc/tpcc_tx_delivery.hh +++ b/include/tpcc/tpcc_tx_delivery.hh @@ -149,8 +149,8 @@ bool update_order_line_and_get_ol_total(TxExecutor& tx, uint16_t w_id, status = tx.update(Storage::OrderLine, ol_key, TupleBody(ol_key, std::move(ol_obj))); - if (status != Status::OK) ERR; if (tx.status_ == TransactionStatus::aborted) { return false; } + if (status != Status::OK) ERR; ol_total += ol.OL_AMOUNT; } return true;