phongn commented on code in PR #13380:
URL: https://github.com/apache/trafficserver/pull/13380#discussion_r3972122705
##########
src/iocore/cache/RamCacheLRU.cc:
##########
@@ -213,6 +218,26 @@ RamCacheLRU::put(CryptoHash *key, IOBufferData *data,
[[maybe_unused]] uint32_t
if (e->auxkey == auxkey) {
lru.remove(e);
lru.enqueue(e);
+ if (copy) {
Review Comment:
Done in f3286882a7 — LRU and S3-FIFO both guard on `copy && !e->copy` now,
so the refresh is reserved for the shared-to-private transition. Thanks; this
race also corrects my framing from the last round. I'd described the refresh
branch as unreachable, but that's true only of the shared-to-private
*transition*. The branch itself fires on exactly the concurrent-miss case you
describe, which is why the guard has real value.
I also applied it to CLFUS, since its pre-existing resident path has the
same redundant copy — it replaces `e->data` unconditionally on every re-put,
both arms. Under the default seen filter it's actually the most exposed of the
three: CLFUS gates its filter on new entries only and S3-FIFO has none, so both
hit the refresh on the second racing put, while LRU filters *before* the
resident walk and toggles per slot, so it needs a fourth.
The CLFUS guard is narrower, though: `copy && e->flag_bits.copy &&
!e->flag_bits.compressed`. The plain form is unsafe there. A compressed entry
holds `compressed_len` bytes behind `flag_bits.copy`, and the resident path
clears `flag_bits.compressed` after the swap — skipping the swap while still
clearing the flag would have `get()` memcpy `e->len` bytes out of a
`compressed_len`-byte buffer (`compressed_len < e->len` is the compressor's
admission condition). So a compressed entry still takes the swap, which is what
decompresses it. The narrow guard is exact rather than approximate: every path
that produces an uncompressed private entry sets `size == len` (Linsert; the
compressor's no-gain branch; `get()`'s in-place decompress is `!copy`-only), so
the `delta` it skips is 0 and the flag writes it skips are no-ops. Only the
allocation and memcpy go away.
One honest caveat: the `!compressed` term isn't covered by a unit test. It
needs a compressed entry, which requires the background compressor on
`ET_TASK`, and that isn't reachable from the Catch2 harness. It rests on the
reasoning above. In the race itself the entry is freshly inserted and the
compressor hasn't run, so the term is there for correctness rather than for the
common path.
The guard is invisible to the gauge — a private copy is already charged
`len`, so `delta` is 0 either way — so the new test pins it by re-putting
*different* bytes under the same key and auxkey and checking the cache kept the
first, for all three policies. Real callers never do that (same key and auxkey
name one on-disk doc), but it's the only observable. It also confirms the
shared-to-private refresh still happens, so the guard is on the entry's state
and not on how many puts it has seen. Disabling any one of the three guards
fails that test for its policy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
