> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Friday, April 18, 2025 8:25 PM
> To: Chia-Yu Chang (Nokia) <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Koen
> De Schepper (Nokia) <[email protected]>; g.white
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; vidhi_goel <[email protected]>
> Subject: Re: [PATCH v4 net-next 10/15] tcp: accecn: AccECN option send control
>
>
> CAUTION: This is an external email. Please be very careful when clicking
> links or opening attachments. See the URL nok.it/ext for additional
> information.
>
>
>
> On Fri, Apr 18, 2025 at 06:34:07PM +0100, Simon Horman wrote:
> > On Fri, Apr 18, 2025 at 01:00:24AM +0200, [email protected]
> > wrote:
> > > From: Ilpo Järvinen <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Co-developed-by: Chia-Yu Chang <[email protected]>
> > > Signed-off-by: Chia-Yu Chang <[email protected]>
> > > ---
> > > 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmirr
> > ors.edge.kernel.org%2Fpub%2Ftools%2Fcrosstool%2Ffiles%2Fbin%2Fx86_64%2
> > F14.2.0%2F&data=05%7C02%7Cchia-yu.chang%40nokia-bell-labs.com%7Cd00e29
> > 455c5a42475ae408dd7ea658e2%7C5d4717519675428d917b70f44f9630b0%7C0%7C0%
> > 7C638805975110989177%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsI
> > lYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C
> > 0%7C%7C%7C&sdata=jYRGDvf%2Bx7lGuSxtgGfredGRNZYDdmlxpiUnwQr4ICQ%3D&rese
> > rved=0
>
> --
> pw-bot: changes-requested
Hi Simon,
Thanks for the tip, I just fixed it based on pahole results and
resubmit v5.
BRs,
Chia-Yu
