Re: [PATCH v1 net-next 8/8] selftest: packetdrill: Add test for tcp_collapse().

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not accurately describe the test in
  tools/testing/selftests/net/packetdrill/tcp_collapse.pkt.

--

selftest: packetdrill: Add test for tcp_collapse().

This adds a packetdrill test, tcp_collapse.pkt. It checks that
tcp_collapse() only collapses runs of low-density skbs, and leaves
high-density skbs and an isolated low-density skb alone. It also makes
ksft_runner.sh skip any script whose PAGE_SIZE define does not match
getconf PAGESIZE.

> tcp_collapse() now does not collapse high-density skbs and a
> low-density skb sandwitched by hihg-density skbs.
>
> tcp_collapse.pkt verifies the behaviour by
>
> 1. set a large SO_RCVBUF
> 2. send one high-density skb with 100 bytes read()
> 3. send one high-density skb
> 4. send one low-density skb
> 5. send one high-density skb
> 6. send 30 low-density skbs
> 7. set a small SO_RCVBUF
> 8. send a small data to trigger tcp_collapse()
> 9. check TcpExtTCPRcvCollapsed via nstat
>
> Without the series, the first high-density skb is collapsed.
> In addition, the first low-density skb drags the following
> high-density skb into collapsing, consuming more memory.
>
> If we remove sk_forced_mem_schedule() without this series,
> sk->sk_forwad_alloc becomes negative at the end of tcp_collapse().

[Severity: Low]
Is a step missing from this list? Between steps 6 and 7,
tcp_collapse.pkt queues one more 62,000 B high-density segment:

// 5. Queue a trailing high-density 62,000 B segment.
   +0 < P. 217001:279001(62000) ack 1 win 65535
    * > .  1:1(0) ack 279001

The file header says the test checks that tcp_collapse() "skips leading
and trailing high-density skbs". The SO_RCVBUF shrink comment gives
sk_rmem_alloc as 334,592, and that figure seems to count four
high-density skbs, not three.

The last paragraph says sk_forward_alloc goes negative if
sk_forced_mem_schedule() is removed without the series. Does this test
catch that case?

The test only passes or fails on three checks:

- TcpExtTCPRcvCollapsed equals 30
- TcpExtTCPRcvQDrop does not appear
- the final read() returns 279900:

   +0 read(4, ..., 300000) = 279900

None of these checks sk_forward_alloc. Could the message say that the
test does not cover this case?

The message also has a few typos: sandwitched (sandwiched), hihg-density
(high-density), and sk_forwad_alloc (sk_forward_alloc).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114040101.1043546-1-kuniyu%40google.com