| Message ID | 20240204104601.55760-1-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add more drop reasons in tcp receive path | expand |
On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > From: Jason Xing <kernelxing@tencent.com> > > When I was debugging the reason about why the skb should be dropped in > syn cookie mode, I found out that this NOT_SPECIFIED reason is too > general. Thus I decided to refine it. Hello, any suggestions? Those names in the patchset could be improper, but I've already tried to name them in English :S Thanks, Jason > > Jason Xing (2): > tcp: add more DROP REASONs in cookie check > tcp: add more DROP REASONS in child process > > include/net/dropreason-core.h | 18 ++++++++++++++++++ > include/net/tcp.h | 8 +++++--- > net/ipv4/syncookies.c | 18 ++++++++++++++---- > net/ipv4/tcp_input.c | 19 +++++++++++++++---- > net/ipv4/tcp_ipv4.c | 13 +++++++------ > net/ipv4/tcp_minisocks.c | 4 ++-- > net/ipv6/tcp_ipv6.c | 6 +++--- > 7 files changed, 64 insertions(+), 22 deletions(-) > > -- > 2.37.3 >
On Sun, 4 Feb 2024 18:45:58 +0800 Jason Xing wrote: > When I was debugging the reason about why the skb should be dropped in > syn cookie mode, I found out that this NOT_SPECIFIED reason is too > general. Thus I decided to refine it. Please run checkpatch --strict on the patches, post v2 with warnings fixed.
On Wed, Feb 7, 2024 at 3:24 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > From: Jason Xing <kernelxing@tencent.com> > > > > When I was debugging the reason about why the skb should be dropped in > > syn cookie mode, I found out that this NOT_SPECIFIED reason is too > > general. Thus I decided to refine it. > > Hello, any suggestions? Those names in the patchset could be improper, > but I've already tried to name them in English :S > Adding &drop_reason parameters all over the places adds more code for CONFIG_STACKPROTECTOR=y builds, because of added canary checks. Please make sure not to slow down the TCP fast path, while we work hard in the opposite direction. Also, sending patch series over weekends increases the chance for them being lost, just my personal opinion... > Thanks, > Jason > > > > > Jason Xing (2): > > tcp: add more DROP REASONs in cookie check > > tcp: add more DROP REASONS in child process > > > > include/net/dropreason-core.h | 18 ++++++++++++++++++ > > include/net/tcp.h | 8 +++++--- > > net/ipv4/syncookies.c | 18 ++++++++++++++---- > > net/ipv4/tcp_input.c | 19 +++++++++++++++---- > > net/ipv4/tcp_ipv4.c | 13 +++++++------ > > net/ipv4/tcp_minisocks.c | 4 ++-- > > net/ipv6/tcp_ipv6.c | 6 +++--- > > 7 files changed, 64 insertions(+), 22 deletions(-) > > > > -- > > 2.37.3 > >
On Wed, Feb 7, 2024 at 11:03 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 4 Feb 2024 18:45:58 +0800 Jason Xing wrote: > > When I was debugging the reason about why the skb should be dropped in > > syn cookie mode, I found out that this NOT_SPECIFIED reason is too > > general. Thus I decided to refine it. > > Please run checkpatch --strict on the patches, post v2 with warnings > fixed. Thanks, I'll do that.
On Wed, Feb 7, 2024 at 5:22 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Feb 7, 2024 at 3:24 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > On Sun, Feb 4, 2024 at 6:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > > From: Jason Xing <kernelxing@tencent.com> > > > > > > When I was debugging the reason about why the skb should be dropped in > > > syn cookie mode, I found out that this NOT_SPECIFIED reason is too > > > general. Thus I decided to refine it. > > > > Hello, any suggestions? Those names in the patchset could be improper, > > but I've already tried to name them in English :S > > > > Adding &drop_reason parameters all over the places adds more code for > CONFIG_STACKPROTECTOR=y builds, > because of added canary checks. Indeed. It took me some while to consider whether I should add more reasons into the fast path. But for now the NOT_SPECIFIED fake reason does not work if we really want to know some useful hints. What do you think? Should I give up this patch series or come up with other better ideas? > > Please make sure not to slow down the TCP fast path, while we work > hard in the opposite direction. I tested some times by using netperf, it's not that easy to observe the obvious differences before/after this patch applied. > > Also, sending patch series over weekends increases the chance for them > being lost, just my personal opinion... Oh, I see :S Thanks, Jason > > > > Thanks, > > Jason > > > > > > > > Jason Xing (2): > > > tcp: add more DROP REASONs in cookie check > > > tcp: add more DROP REASONS in child process > > > > > > include/net/dropreason-core.h | 18 ++++++++++++++++++ > > > include/net/tcp.h | 8 +++++--- > > > net/ipv4/syncookies.c | 18 ++++++++++++++---- > > > net/ipv4/tcp_input.c | 19 +++++++++++++++---- > > > net/ipv4/tcp_ipv4.c | 13 +++++++------ > > > net/ipv4/tcp_minisocks.c | 4 ++-- > > > net/ipv6/tcp_ipv6.c | 6 +++--- > > > 7 files changed, 64 insertions(+), 22 deletions(-) > > > > > > -- > > > 2.37.3 > > >
On Wed, Feb 7, 2024 at 2:25 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > Indeed. > > It took me some while to consider whether I should add more reasons > into the fast path. > > But for now the NOT_SPECIFIED fake reason does not work if we really > want to know some useful hints. > What do you think? Should I give up this patch series or come up with > other better ideas? Perhaps find a way to reuse return values from functions to carry a drop_reason. > > > > > Please make sure not to slow down the TCP fast path, while we work > > hard in the opposite direction. > > I tested some times by using netperf, it's not that easy to observe > the obvious differences before/after this patch applied. Sure, the difference is only noticeable on moderate load, when a cpu receives one packet in a while. icache pressure, something hard to measure with synthetic benchmarks, but visible in real workloads in the long term. At Google, we definitely see an increase of network cpu costs releases after releases.
On Wed, Feb 7, 2024 at 9:37 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Feb 7, 2024 at 2:25 PM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > > Indeed. > > > > It took me some while to consider whether I should add more reasons > > into the fast path. > > > > But for now the NOT_SPECIFIED fake reason does not work if we really > > want to know some useful hints. > > What do you think? Should I give up this patch series or come up with > > other better ideas? > > Perhaps find a way to reuse return values from functions to carry a drop_reason. It seems feasible to reuse return values, let me work on it :) > > > > > > > > > Please make sure not to slow down the TCP fast path, while we work > > > hard in the opposite direction. > > > > I tested some times by using netperf, it's not that easy to observe > > the obvious differences before/after this patch applied. > > Sure, the difference is only noticeable on moderate load, when a cpu > receives one packet in a while. > > icache pressure, something hard to measure with synthetic benchmarks, > but visible in real workloads in the long term. > > At Google, we definitely see an increase of network cpu costs releases > after releases. Thanks for your explanations. Thanks, Jason
From: Jason Xing <kernelxing@tencent.com> When I was debugging the reason about why the skb should be dropped in syn cookie mode, I found out that this NOT_SPECIFIED reason is too general. Thus I decided to refine it. Jason Xing (2): tcp: add more DROP REASONs in cookie check tcp: add more DROP REASONS in child process include/net/dropreason-core.h | 18 ++++++++++++++++++ include/net/tcp.h | 8 +++++--- net/ipv4/syncookies.c | 18 ++++++++++++++---- net/ipv4/tcp_input.c | 19 +++++++++++++++---- net/ipv4/tcp_ipv4.c | 13 +++++++------ net/ipv4/tcp_minisocks.c | 4 ++-- net/ipv6/tcp_ipv6.c | 6 +++--- 7 files changed, 64 insertions(+), 22 deletions(-)