On Tue, Mar 24, 2026 at 12:01:23PM +0100, Laurent Vivier wrote:
> tcp_fill_headers() takes a pointer to a previously computed IPv4 header
> checksum to avoid recalculating it when the payload length doesn't
> change, and a separate bool to skip TCP checksum computation.
> 
> Replace both parameters with a single uint32_t csum_flags that encodes:
> - IP4_CSUM (bit 31): compute IPv4 header checksum from scratch
> - TCP_CSUM (bit 30): compute TCP checksum
> - IP4_CMASK (low 16 bits): cached IPv4 header checksum value
> 
> When IP4_CSUM is not set, the cached checksum is extracted from the low
> 16 bits.  This is cleaner than the pointer-based approach, and also
> avoids a potential dangling pointer issue: a subsequent patch makes
> tcp_fill_headers() access ip4h via with_header(), which scopes it to a
> temporary variable, so a pointer to ip4h->check would become invalid
> after the with_header() block.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

When I suggested this, I'd missed the fact that @ip4_check and
@no_tcp_csum were talking about different checksums, oops.
Nonetheless you've made it work :).

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

[snip]
>  	if (ip4h)
> -		*check = &ip4h->check;
> +		*csum_flags = (*csum_flags & TCP_CSUM) | ip4h->check;

Now that I've realised my mistake, I'm pretty neutral on whether we
include the TCP checksum control in this parameter.  I still think
avoiding the pointer is a significant win - not referencing one
packet's buffer when we're working on another means less non-obvious
constraints in how we organise those buffers.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson