On 11/3/26 11:22, Paolo Abeni wrote:
On 11/2/26 19:24, netdev-bot+sashiko@kernel.org wrote:
quoted
- [Medium] The patch adds a cons.count < peer_rmbe_size gate to
smc_switch_cursor().
- [Medium] The new bound in smc_switch_cursor(), diff <=
smc_curs_diff(len, &tx_curs_sent, &prep), is computed from a snapshot...
I think the above are the last 2 comments standing that need to be addressed,
(the high one in patch 1 is addressed by patch 2, the high one in patch 2 is
addressed by a separate patch).
Both are right; v9 [1] changes the code for each.
quoted
- [Medium] The patch adds a cons.count < peer_rmbe_size gate to
smc_switch_cursor().
For a count past the RMBE, v8 turned a stalled connection into a write
past the peer's RMBE. A check in smc_write_space() isn't enough:
smc_cdc_msg_recv_action() credits peer_rmbe_space from the next
consumer cursor, so one more CDC after the switch opens the window
again. A conforming peer gets the same write if it keeps reading across
the end of its RMBE before the resend.
v9 bounds the write where its length and destination are computed (new
5/6): at most one RMBE per call, and the first chunk ends no later than
the RMBE whatever the wraps say. On rxe with a 16K RMBE and the count
forged at RMBE + 4096, v8 posts 16384 bytes from offset 1 and the link
goes down; v9 posts 16383 + 1 and the link stays up. With a conforming
peer, 9000 bytes unconfirmed at the failover and read before the
resend, then 2000 more sent, unpatched and v8 post the 11000 bytes in
one chunk from offset 12001 and the surviving link goes down; v9 posts
4383 + 6617 and the peer reads the stream intact.
5/6 turns one-chunk writes into two, so it sits on a fix for how
smcr_tx_rdma_writes() sizes the second chunk's source (new 4/6, the
pre-existing [High] on the source SGE in the review of v7's 4/4). A
conforming peer reaches that one: a completion handled between the
first two cursor copies of smc_switch_cursor() leaves tx_curs_sent 600
bytes behind prod, and unpatched and v8 then build a source SGE that
ends 4536 bytes past the send buffer (KASAN splat on the unpatched
run).
quoted
- [Medium] The new bound in smc_switch_cursor(), diff <=
smc_curs_diff(len, &tx_curs_sent, &prep), is computed from a snapshot...
6/6 (v8's 4/4) sets tx_curs_fin and local_tx_ctrl_fin from the advanced
tx_curs_sent and local_tx_ctrl.prod, which change only under send_lock,
instead of adding to the live cursors: the first of the review's two
options. With a completion handed to smc_cdc_tx_handler() inside the
switch (test hook), v8 leaves local_tx_ctrl_fin at count 16713 for a
16384-byte RMBE; v9 leaves it on prod.
The switch still isn't serialized against smc_cdc_tx_handler(): a
completion handled inside it is still credited to sndbuf_space more
than once (the pre-existing [High] on that race) and can make the next
write hand the peer bytes it has already read. 3/6 keeps send() inside
the send buffer, but send() can then overwrite data the peer has not
received (by inspection). Serializing the switch means taking the
socket spinlock around send_lock in smc_switch_conns(); v9 doesn't do
that. In two races with a completion inside the switch v9 ends worse
than unpatched; the cover has both.
The [Low] of that review, on the new comment in smc_switch_cursor():
6/6's comment no longer says "else that peer stalls", and the message
no longer says "still sizes".
[1] https://lore.kernel.org/all/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me/ (local)
Thanks,
Bryam