Re: [PATCH v1 net-next 3/8] tcp: Stop collapsing if not bloated.

[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 header comment on tcp_collapse() in net/ipv4/tcp_input.c
  (line 5841-5842) was edited by this patch to read "Collapse contiguous…

--

tcp: Stop collapsing if not bloated.

This commit changes tcp_collapse() so it only collapses ranges of bloated
skbs. It no longer starts collapsing because the next skb overlaps, and it
no longer runs on until the tail. The end parameter is dropped. A local
sub_end is computed for each bloated range, and the function restarts the
search after each range.

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index e07f433474093..5bd42a74c6f58 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5839,7 +5839,7 @@ static struct sk_buff *tcp_collapse_one(struct sock *sk, struct sk_buff *skb,
>  }
>  
>  /* Collapse contiguous sequence of skbs head..tail with
> - * sequence numbers start..end.
> + * sequence numbers start..sub_end.

[Severity: Low]
This isn't a bug, but should this header comment for tcp_collapse() be
reworded rather than just renamed?

sub_end is a local variable, not a parameter. The second loop recomputes
it for each bloated sub-range, and it changes after every goto restart.
Someone reading the function's contract won't know what sub_end refers to.

The comment also still says head..tail is collapsed as one contiguous run.
With this patch, non-bloated skbs are skipped, each sub-range stops at the
first non-bloated, non-overlapping skb, and only the bloated sub-ranges
inside head..tail are collapsed.

The comment above the caller, tcp_collapse_ofo_queue(), has the same
mismatch:

/* Collapse ofo queue. Algorithm: select contiguous sequence of skbs
 * and tcp_collapse() them until all the queue is collapsed.
 */

The last patch in the series still leaves the header comment as
start..sub_end.

>   *
>   * If tail is NULL, this means until the end of the queue.
>   *
> @@ -5848,15 +5848,18 @@ static struct sk_buff *tcp_collapse_one(struct sock *sk, struct sk_buff *skb,
>   */
>  static void
>  tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
> -	     struct sk_buff *head, struct sk_buff *tail, u32 start, u32 end)
> +	     struct sk_buff *head, struct sk_buff *tail, u32 start)
>  {
>  	struct sk_buff *skb = head, *n;
>  	struct sk_buff_head tmp;
>  	u8 scaling_ratio;
> +	u32 sub_end;

[ ... ]

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