> -----Original Message----- > From: Simon Horman <[email protected]> > Sent: Wednesday, April 16, 2025 11:07 AM > 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]; 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 v3 net-next 05/15] tcp: accecn: add AccECN rx byte > counters > > > 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 Mon, Apr 14, 2025 at 03:13:05PM +0200, [email protected] > wrote: > > From: Ilpo Järvinen <[email protected]> > > > > These counters track IP ECN field payload byte sums for all arriving > > (acceptable) packets. The AccECN option (added by a later patch in the > > series) echoes these counters back to sender side. > > > > Signed-off-by: Ilpo Järvinen <[email protected]> > > Signed-off-by: Neal Cardwell <[email protected]> > > Signed-off-by: Chia-Yu Chang <[email protected]> > > --- > > include/linux/tcp.h | 1 + > > include/net/tcp.h | 18 +++++++++++++++++- > > net/ipv4/tcp.c | 3 ++- > > net/ipv4/tcp_input.c | 13 +++++++++---- > > net/ipv4/tcp_minisocks.c | 3 ++- > > 5 files changed, 31 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h index > > af38fff24aa4..9cbfefd693e3 100644 > > --- a/include/linux/tcp.h > > +++ b/include/linux/tcp.h > > @@ -303,6 +303,7 @@ struct tcp_sock { > > u32 delivered; /* Total data packets delivered incl. rexmits > > */ > > u32 delivered_ce; /* Like the above but only ECE marked packets > > */ > > u32 received_ce; /* Like the above but for rcvd CE marked pkts > > */ > > + u32 received_ecn_bytes[3]; > > u8 received_ce_pending:4, /* Not yet transmit cnt of received_ce > > */ > > unused2:4; > > u32 app_limited; /* limited until "delivered" reaches this val > > */ > > ... > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index > > 73f8cc715bff..278990dba721 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -5092,6 +5092,7 @@ static void __init tcp_struct_check(void) > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, tcp_sock_write_txrx, > > delivered); > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, tcp_sock_write_txrx, > > delivered_ce); > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, > > tcp_sock_write_txrx, received_ce); > > + CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, > > + tcp_sock_write_txrx, received_ecn_bytes); > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, tcp_sock_write_txrx, > > app_limited); > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, tcp_sock_write_txrx, > > rcv_wnd); > > CACHELINE_ASSERT_GROUP_MEMBER(struct tcp_sock, > > tcp_sock_write_txrx, rx_opt); @@ -5099,7 +5100,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, 97 > > + 7); > > + CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, > > + tcp_sock_write_txrx, 109 + 3); > > Hi Ilpo, > > I think that incrementing 97 to 109 is correct, as 12 bytes have been added > to this group. > > However, I do not think it is correct to decrement 7 to 3. > > On, at least, x86_64, x86_32 and arm64 that decrement does not cause any > problems. (I assume it is also fine without the rest of this patch). > > But on (32-bit) ARM, this causes the assertion to fail. > This is because on ARM an extra 4-byte hole is added just after pred_flags. > And the assertion checks an upper bound on the size of the group. > > I assume this extra hole is due to alignment requirements. > In any case on ARM, with this patch applied, pahole shows the group looking > like this: > > __u8 > __cacheline_group_begin__tcp_sock_write_txrx[0]; /* 1869 0 */ > > /* XXX 3 bytes hole, try to pack */ > > __be32 pred_flags; /* 1872 4 */ > > /* XXX 4 bytes hole, try to pack */ > > u64 tcp_clock_cache; /* 1880 8 */ > u64 tcp_mstamp; /* 1888 8 */ > u32 rcv_nxt; /* 1896 4 */ > u32 snd_nxt; /* 1900 4 */ > u32 snd_una; /* 1904 4 */ > u32 window_clamp; /* 1908 4 */ > u32 srtt_us; /* 1912 4 */ > u32 packets_out; /* 1916 4 */ > /* --- cacheline 30 boundary (1920 bytes) --- */ > u32 snd_up; /* 1920 4 */ > u32 delivered; /* 1924 4 */ > u32 delivered_ce; /* 1928 4 */ > u32 received_ce; /* 1932 4 */ > u32 received_ecn_bytes[3]; /* 1936 12 */ > u8 received_ce_pending:4; /* 1948: 0 1 */ > u8 unused2:4; /* 1948: 4 1 */ > > /* XXX 3 bytes hole, try to pack */ > > u32 app_limited; /* 1952 4 */ > u32 rcv_wnd; /* 1956 4 */ > struct tcp_options_received rx_opt; /* 1960 24 */ > /* --- cacheline 31 boundary (1984 bytes) --- */ > u8 nonagle:4; /* 1984: 0 1 */ > u8 rate_app_limited:1; /* 1984: 4 1 */ > > /* XXX 3 bits hole, try to pack */ > > __u8 > __cacheline_group_end__tcp_sock_write_txrx[0]; /* 1985 0 */ > > So we are now at 3 cache lines. And perhaps it is worth trying to pack things > a bit. Or perhaps that becomes tricky to get right across different > architectures. I didn't explore that. > > But, taking the naive approach: with the following update, tcp.c compiles > compiles with allmodconfig on x86_64, x86_32, ARM and arm64 (I did not test > others). > > CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 109 > + 7); > > For the record, gcc 14.2.0 reports this problem as: > > CC net/ipv4/tcp.o > In file included from <command-line>: > In function 'tcp_struct_check', > inlined from 'tcp_init' at net/ipv4/tcp.c:5133:2: > ././include/linux/compiler_types.h:557:45: error: call to > '__compiletime_assert_ 1403' declared with attribute error: BUILD_BUG_ON > failed: offsetof(struct tcp_sock, __cacheline_group_end__tcp_sock_write_txrx) > - offsetofend(struct tcp_sock, __cacheline_group_begin__tcp_sock_write_txrx) > > 109 + 3 > 557 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | ^ > ././include/linux/compiler_types.h:538:25: note: in definition of macro > '__compiletime_assert' > 538 | prefix ## suffix(); > \ > | ^~~~~~ > ././include/linux/compiler_types.h:557:9: note: in expansion of macro > '_compiletime_assert' > 557 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:39:37: note: in expansion of macro > 'compiletime_assert' > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > | ^~~~~~~~~~~~~~~~~~ > ./include/linux/build_bug.h:50:9: note: in expansion of macro > 'BUILD_BUG_ON_MSG' > 50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " > #condition) > | ^~~~~~~~~~~~~~~~ > ./include/linux/cache.h:160:9: note: in expansion of macro 'BUILD_BUG_ON' > 160 | BUILD_BUG_ON(offsetof(TYPE, __cacheline_group_end__##GROUP) - > \ > | ^~~~~~~~~~~~ > net/ipv4/tcp.c:5103:9: note: in expansion of macro > 'CACHELINE_ASSERT_GROUP_SIZE' > 5103 | CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, > tcp_sock_write_txrx, 109 + 3); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ...
Thanks Simon for pointing out this issue, my previous calculation is without considering that for ARM one extra 4-byte hole is added. I will modify and resubmit in the next version. Chia-Yu > > -- > pw-bot: changes-requested
