Loading src/quic.cpp +43 −21 Original line number Diff line number Diff line Loading @@ -7843,28 +7843,35 @@ size_t quic::sendStreamDataEarly(uint64_t stream_id, const uint8_t* data, size_t // ============================================================================ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, bool fin) { // F30 (performance report): deliberately NOT implemented. The report // suggests moving AES-GCM encryption outside the quic_mtx() lock and // only holding PN reservation and _sent_packets/cwnd bookkeeping under // a short lock, to reduce the measured contention between an // application thread sending and the network thread concurrently // processing ACKs for the same connection. // F30 (performance report), implemented 2026-08-15: moves the AES-GCM // and header-protection AES-ECB calls in the per-packet loop below // outside quic_mtx(), so a concurrent thread processing incoming ACKs // for this same connection doesn't have to wait for this packet's // crypto to finish just to acquire quic_mtx(). PN reservation, the // STREAM/ACK frame build (which reads/clears _app_ack_pending and // stream.send_offset), and the post-encrypt batchPacket()/ // recordSentPacket()/onPacketSentCC()/offset-advance bookkeeping all // stay under quic_mtx() exactly as before -- only the pure-computation // crypto calls (operating on this thread's own stack buffers // pkt_buf/pt_buf_base plus immutable per-connection key material; aes // instances are documented safe for concurrent calls across threads as // long as output buffers don't overlap, see aes.h) run unlocked. // // That would require restructuring PN reservation, encryption, and the // actual sendPacket() syscall to run under different lock granularity, // while still strictly preserving on-wire PN ordering and // _sent_packets consistency. A race slipped in here would, with high // probability, NOT show up on this loopback test system (near-zero // latency, hardly any real queuing), but could show up exactly in the // production scenario with real latency/jitter that this fix is meant // to improve (packet reordering, corrupted retransmission bookkeeping). // Effort and risk are rated "high"/"high" in the report itself — // without multi-core/variable-bandwidth validation outside this // sandbox, the risk of a silent, hard-to-reproduce bug in the critical // send path (which F31 just finished verifying correct via 3x full // test suite + 3x quic_concurrent_test) outweighs the gain. Decision: // not implemented, documented transparently instead of forced through // at risk. // This loop was, and remains, strictly sequential on whichever thread // calls sendStreamData() -- packet N+1's build phase never starts // before packet N's send phase has completed -- so releasing // quic_mtx() mid-packet does not by itself risk reordering (the same // thread just isn't holding that particular lock for a moment; nothing // about the loop's own progress changes). The one real new risk it // would open is a *second* thread calling sendStreamData() on this same // connection concurrently, slipping its own packet in between this // one's unlock and relock and reaching batchPacket() first with a // higher packet number -- exactly the "packet reordering" failure this // change was originally rejected over. _send_pipeline_mutex (see its // declaration in socket.h) closes that: held for this whole per-packet // sequence, it keeps packets reaching batchPacket()/recordSentPacket() // in strict packet-number order even if sendStreamData() is ever called // from more than one thread for the same connection. std::unique_lock<std::recursive_mutex> lock(quic_mtx()); // 0-RTT: the handshake hasn't completed, but early data keys are ready Loading Loading @@ -8078,6 +8085,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, if (!unblocked) break; } // F30: serializes this packet's whole build->encrypt->send // sequence against any other thread doing the same on this // connection -- see its declaration and this function's own // comment above. std::lock_guard<std::recursive_mutex> pipeline_lock(_send_pipeline_mutex); // === Build short header directly into pkt_buf === uint8_t first_byte = 0x40 | 0x03; // Short header, 4-byte PN pkt_buf[0] = first_byte; Loading Loading @@ -8184,6 +8197,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, uint8_t* ct_dst = pkt_buf + hdr_len; uint8_t* tag_dst = ct_dst + payload_len; // F30: quic_mtx() released for the two crypto calls below (see this // function's opening comment) -- pipeline_lock (_send_pipeline_mutex) // stays held throughout, preserving this packet's send ordering // relative to any other thread doing the same on this connection. lock.unlock(); if (cached_aes) { cached_aes->aes_gcm_encrypt(nonce, pkt_buf, hdr_len, pt_buf_base, payload_len, Loading @@ -8207,6 +8226,9 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, aes128 tmp_hp(std::vector<uint8_t>(hp_key, hp_key + 16)); tmp_hp.encrypt_ecb(sample, hp_mask); } lock.lock(); pkt_buf[0] ^= (hp_mask[0] & 0x1F); // Short header mask size_t pn_offset = 1 + cid_len; for (size_t i = 0; i < 4; ++i) Loading src/socket.h +17 −0 Original line number Diff line number Diff line Loading @@ -1863,6 +1863,23 @@ namespace netplus { // the ordering avoids relying on that distinction. mutable std::recursive_mutex _quic_mutex; // F30 (performance report, implemented 2026-08-15): guards one // packet's build->encrypt->send sequence in sendStreamData(), which // releases quic_mtx() around the AES-GCM/header-protection calls so // a concurrent thread processing incoming ACKs for this same // connection doesn't have to wait for this packet's crypto just to // acquire quic_mtx(). quic_mtx() alone no longer guarantees // packets reach batchPacket()/recordSentPacket() in packet-number // order once it can be released mid-packet — this mutex (held for // the whole per-packet sequence, from PN reservation through the // post-encrypt bookkeeping) restores that guarantee even if // multiple threads ever call sendStreamData() concurrently on the // same connection. Recursive for the same reentrancy reason as // _quic_mutex above (processFrame callbacks invoked from // pumpIncomingLocked(), itself called while this is held, can // re-enter sendStreamData()). mutable std::recursive_mutex _send_pipeline_mutex; // Guards _child_connections only (registry membership), never a // connection's own state — that's quic_mtx()'s job. shared_mutex // because lookups (every packet routed to an existing connection) Loading Loading
src/quic.cpp +43 −21 Original line number Diff line number Diff line Loading @@ -7843,28 +7843,35 @@ size_t quic::sendStreamDataEarly(uint64_t stream_id, const uint8_t* data, size_t // ============================================================================ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, bool fin) { // F30 (performance report): deliberately NOT implemented. The report // suggests moving AES-GCM encryption outside the quic_mtx() lock and // only holding PN reservation and _sent_packets/cwnd bookkeeping under // a short lock, to reduce the measured contention between an // application thread sending and the network thread concurrently // processing ACKs for the same connection. // F30 (performance report), implemented 2026-08-15: moves the AES-GCM // and header-protection AES-ECB calls in the per-packet loop below // outside quic_mtx(), so a concurrent thread processing incoming ACKs // for this same connection doesn't have to wait for this packet's // crypto to finish just to acquire quic_mtx(). PN reservation, the // STREAM/ACK frame build (which reads/clears _app_ack_pending and // stream.send_offset), and the post-encrypt batchPacket()/ // recordSentPacket()/onPacketSentCC()/offset-advance bookkeeping all // stay under quic_mtx() exactly as before -- only the pure-computation // crypto calls (operating on this thread's own stack buffers // pkt_buf/pt_buf_base plus immutable per-connection key material; aes // instances are documented safe for concurrent calls across threads as // long as output buffers don't overlap, see aes.h) run unlocked. // // That would require restructuring PN reservation, encryption, and the // actual sendPacket() syscall to run under different lock granularity, // while still strictly preserving on-wire PN ordering and // _sent_packets consistency. A race slipped in here would, with high // probability, NOT show up on this loopback test system (near-zero // latency, hardly any real queuing), but could show up exactly in the // production scenario with real latency/jitter that this fix is meant // to improve (packet reordering, corrupted retransmission bookkeeping). // Effort and risk are rated "high"/"high" in the report itself — // without multi-core/variable-bandwidth validation outside this // sandbox, the risk of a silent, hard-to-reproduce bug in the critical // send path (which F31 just finished verifying correct via 3x full // test suite + 3x quic_concurrent_test) outweighs the gain. Decision: // not implemented, documented transparently instead of forced through // at risk. // This loop was, and remains, strictly sequential on whichever thread // calls sendStreamData() -- packet N+1's build phase never starts // before packet N's send phase has completed -- so releasing // quic_mtx() mid-packet does not by itself risk reordering (the same // thread just isn't holding that particular lock for a moment; nothing // about the loop's own progress changes). The one real new risk it // would open is a *second* thread calling sendStreamData() on this same // connection concurrently, slipping its own packet in between this // one's unlock and relock and reaching batchPacket() first with a // higher packet number -- exactly the "packet reordering" failure this // change was originally rejected over. _send_pipeline_mutex (see its // declaration in socket.h) closes that: held for this whole per-packet // sequence, it keeps packets reaching batchPacket()/recordSentPacket() // in strict packet-number order even if sendStreamData() is ever called // from more than one thread for the same connection. std::unique_lock<std::recursive_mutex> lock(quic_mtx()); // 0-RTT: the handshake hasn't completed, but early data keys are ready Loading Loading @@ -8078,6 +8085,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, if (!unblocked) break; } // F30: serializes this packet's whole build->encrypt->send // sequence against any other thread doing the same on this // connection -- see its declaration and this function's own // comment above. std::lock_guard<std::recursive_mutex> pipeline_lock(_send_pipeline_mutex); // === Build short header directly into pkt_buf === uint8_t first_byte = 0x40 | 0x03; // Short header, 4-byte PN pkt_buf[0] = first_byte; Loading Loading @@ -8184,6 +8197,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, uint8_t* ct_dst = pkt_buf + hdr_len; uint8_t* tag_dst = ct_dst + payload_len; // F30: quic_mtx() released for the two crypto calls below (see this // function's opening comment) -- pipeline_lock (_send_pipeline_mutex) // stays held throughout, preserving this packet's send ordering // relative to any other thread doing the same on this connection. lock.unlock(); if (cached_aes) { cached_aes->aes_gcm_encrypt(nonce, pkt_buf, hdr_len, pt_buf_base, payload_len, Loading @@ -8207,6 +8226,9 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, aes128 tmp_hp(std::vector<uint8_t>(hp_key, hp_key + 16)); tmp_hp.encrypt_ecb(sample, hp_mask); } lock.lock(); pkt_buf[0] ^= (hp_mask[0] & 0x1F); // Short header mask size_t pn_offset = 1 + cid_len; for (size_t i = 0; i < 4; ++i) Loading
src/socket.h +17 −0 Original line number Diff line number Diff line Loading @@ -1863,6 +1863,23 @@ namespace netplus { // the ordering avoids relying on that distinction. mutable std::recursive_mutex _quic_mutex; // F30 (performance report, implemented 2026-08-15): guards one // packet's build->encrypt->send sequence in sendStreamData(), which // releases quic_mtx() around the AES-GCM/header-protection calls so // a concurrent thread processing incoming ACKs for this same // connection doesn't have to wait for this packet's crypto just to // acquire quic_mtx(). quic_mtx() alone no longer guarantees // packets reach batchPacket()/recordSentPacket() in packet-number // order once it can be released mid-packet — this mutex (held for // the whole per-packet sequence, from PN reservation through the // post-encrypt bookkeeping) restores that guarantee even if // multiple threads ever call sendStreamData() concurrently on the // same connection. Recursive for the same reentrancy reason as // _quic_mutex above (processFrame callbacks invoked from // pumpIncomingLocked(), itself called while this is held, can // re-enter sendStreamData()). mutable std::recursive_mutex _send_pipeline_mutex; // Guards _child_connections only (registry membership), never a // connection's own state — that's quic_mtx()'s job. shared_mutex // because lookups (every packet routed to an existing connection) Loading