Re: [PATCH] net: tcp_drop adds `reason` parameter for tracing v2

On Wed, Sep 1, 2021 at 7:36 AM Steven Rostedt [off-list ref] wrote:

On Thu, 26 Aug 2021 15:13:07 +1000
Brendan Gregg [off-list ref] wrote:

quoted

On Thu, Aug 26, 2021 at 1:20 PM Steven Rostedt [off-list ref] wrote:

quoted

On Wed, 25 Aug 2021 08:47:46 -0700
Eric Dumazet [off-list ref] wrote:

quoted

quoted

@@ -5703,15 +5700,15 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
                        TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
                NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPSYNCHALLENGE);
                tcp_send_challenge_ack(sk, skb);
-               goto discard;
+               tcp_drop(sk, skb, TCP_DROP_MASK(__LINE__, TCP_VALIDATE_INCOMING));

I'd rather use a string. So that we can more easily identify _why_ the
packet was drop, without looking at the source code
of the exact kernel version to locate line number 1057

You can be sure that we will get reports in the future from users of
heavily modified kernels.
Having to download a git tree, or apply semi-private patches is a no go.

If you really want to include __FILE__ and __LINE__, these both can be
stringified and included in the report, with the help of macros.

I agree the __LINE__ is pointless, but if this has a tracepoint
involved, then you can simply enable the stacktrace trigger to it and
it will save a stack trace in the ring buffer for you.

   echo stacktrace > /sys/kernel/tracing/events/tcp/tcp_drop/trigger

And when the event triggers it will record a stack trace. You can also
even add a filter to do it only for specific reasons.

   echo 'stacktrace if reason == 1' > /sys/kernel/tracing/events/tcp/tcp_drop/trigger

And it even works for flags:

   echo 'stacktrace if reason & 0xa' > /sys/kernel/tracing/events/tcp/tcp_drop/trigger

Which gives another reason to use an enum over a string.

You can't do string comparisons? The more string support Ftrace has,
the more convenient they will be. Using bpftrace as an example of
convenience and showing drop frequency counted by human-readable
reason and stack trace:

Yes, you can (and pretty much always had this ability), but having
flags is usually makes it easier (and faster).

You can have 'stacktrace if reason ~ "*string*"' which will match
anything with "string" in it.

My main argument against strings is more of the space they take up in
the ring buffer than the ability to filter.

Understood the concern about size, but it seems the trace includes many things.
Can we have an estimate of the size needed per event ?
If we do not use symbolic, but numbers, I am afraid this trace event
will only be used by a few TCP experts.

+               TP_printk("src=%pISpc dest=%pISpc mark=%#x data_len=%d
snd_nxt=%#x snd_una=%#x \
+                               snd_cwnd=%u ssthresh=%u snd_wnd=%u
srtt=%u rcv_wnd=%u \
+                               sock_cookie=%llx reason=%d
reason_type=%s reason_line=%d",
                                __entry->saddr, __entry->daddr, __entry->mark,
                                __entry->data_len, __entry->snd_nxt,
__entry->snd_una,
                                __entry->snd_cwnd, __entry->ssthresh,
__entry->snd_wnd,
-                               __entry->srtt, __entry->rcv_wnd,
__entry->sock_cookie, __entry->reason)
+                               __entry->srtt, __entry->rcv_wnd,
__entry->sock_cookie,
+                               __entry->reason,
+                               __print_symbolic(__entry->reason_code,
TCP_DROP_REASON),
+                               __entry->reason_line)
 );