Re: [PATCH v4 net-next 10/15] tcp: accecn: AccECN option send control

[Simon Horman <horms@kernel.org>]

On Fri, Apr 18, 2025 at 06:34:07PM +0100, Simon Horman wrote:
> On Fri, Apr 18, 2025 at 01:00:24AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > From: Ilpo Järvinen <ij@kernel.org>
> > 
> > Instead of sending the option in every ACK, limit sending to
> > those ACKs where the option is necessary:
> > - Handshake
> > - "Change-triggered ACK" + the ACK following it. The
> >   2nd ACK is necessary to unambiguously indicate which
> >   of the ECN byte counters in increasing. The first
> >   ACK has two counters increasing due to the ecnfield
> >   edge.
> > - ACKs with CE to allow CEP delta validations to take
> >   advantage of the option.
> > - Force option to be sent every at least once per 2^22
> >   bytes. The check is done using the bit edges of the
> >   byte counters (avoids need for extra variables).
> > - AccECN option beacon to send a few times per RTT even if
> >   nothing in the ECN state requires that. The default is 3
> >   times per RTT, and its period can be set via
> >   sysctl_tcp_ecn_option_beacon.
> > 
> > Signed-off-by: Ilpo Järvinen <ij@kernel.org>
> > Co-developed-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > ---
> >  include/linux/tcp.h        |  3 +++
> >  include/net/netns/ipv4.h   |  1 +
> >  include/net/tcp.h          |  1 +
> >  net/ipv4/sysctl_net_ipv4.c |  9 ++++++++
> >  net/ipv4/tcp.c             |  5 ++++-
> >  net/ipv4/tcp_input.c       | 36 +++++++++++++++++++++++++++++++-
> >  net/ipv4/tcp_ipv4.c        |  1 +
> >  net/ipv4/tcp_minisocks.c   |  2 ++
> >  net/ipv4/tcp_output.c      | 42 ++++++++++++++++++++++++++++++--------
> >  9 files changed, 90 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index 0e032d9631ac..9619524d8901 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -309,7 +309,10 @@ struct tcp_sock {
> >  	u8	received_ce_pending:4, /* Not yet transmit cnt of received_ce */
> >  		unused2:4;
> >  	u8	accecn_minlen:2,/* Minimum length of AccECN option sent */
> > +		prev_ecnfield:2,/* ECN bits from the previous segment */
> > +		accecn_opt_demand:2,/* Demand AccECN option for n next ACKs */
> >  		est_ecnfield:2;/* ECN field for AccECN delivered estimates */
> > +	u64	accecn_opt_tstamp;	/* Last AccECN option sent timestamp */
> >  	u32	app_limited;	/* limited until "delivered" reaches this val */
> >  	u32	rcv_wnd;	/* Current receiver window		*/
> >  /*
> 
> ...
> 
> > @@ -5113,7 +5116,7 @@ static void __init tcp_struct_check(void)
> >  	/* 32bit arches with 8byte alignment on u64 fields might need padding
> >  	 * before tcp_clock_cache.
> >  	 */
> > -	CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 122 + 6);
> > +	CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 130 + 6);
> 
> Hi,
> 
> While this seems find on x86_64, x86_32 and arm64, it does not seem correct
> on ARM (32-bit).
> 
> This is because the existing two byte hole after est_ecnfield grows
> to 6 bytes. I assume this is because of alignment requirements.
> But in any case, the result is that the overall group size increases
> by 12 bytes rather than 8.
> 
> I believe that you can avoid the hole growing, and thus limit the overall
> increase in size of the group to 12 bytes, by placing accecn_opt_tstamp
> elsewhere, e.g. after app_limited rather than before it.
> 
> You can exercise this by cross compiling for ARM and examining
> the structure layout using pahole.
> 
> Cross compilers available from [1] should be able to do that something
> like this:
> 
> PATH=".../gcc-14.2.0-nolibc/arm-linux-gnueabi/bin:$PATH"
> export ARCH=arm
> export CROSS_COMPILE=arm-linux-gnueabi-
> 
> make allmodconfig
> echo "CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y" >> .config
> yes "" | make oldconfig
> 
> make net/ipv4/tcp.o

Sorry, I omitted the invocation of pahole.

pahole net/ipv4/tcp.o

> [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/14.2.0/

-- 
pw-bot: changes-requested