On Tue, Sep 3, 2024 at 2:40 PM Jakub Kicinski kuba@kernel.org wrote:
On Sat, 31 Aug 2024 00:43:08 +0000 Mina Almasry wrote:
static inline bool tcp_skb_can_collapse_to(const struct sk_buff *skb) {
return likely(!TCP_SKB_CB(skb)->eor);
return likely(!TCP_SKB_CB(skb)->eor && skb_frags_readable(skb));Do you remember why this is here?
Yeah, to be honest, when I first implemented some of these checks I erred on the side of caution, and added checks around anything that looked concerning, some of these unnecessary checks got removed, but looks like this one didn't.
Both for Rx and Tx what should matter is whether the "readability" matches, right? We can merge two unreadable messages.
Yes, you're right, only 'readability matches' should be the criteria here. `tcp_skb_can_collapse` already checks readability is matching correctly, so no issue there. The `tcp_skb_can_collapse_to` check you're commenting on here looks unnecessary. I will remove it and run that through some testing.
As an aside, it looks to me like that tcp_skb_can_collapse_to callsites don't seem to be doing any collapsing. Unless I misread the code. It looks like tcp_skb_can_collapse_to is used as an eor check. I can rename the function to tcp_skb_is_eor() or something if that makes sense (in a separate patch). I think the name of the function confused me slightly and made me think I need to do a readability check.
-- Thanks, Mina