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

On September 1, 2021 11:20:58 AM EDT, Eric Dumazet [off-list ref] wrote:

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

quoted

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

quoted

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,

quoted

quoted

quoted

quoted

quoted

                        TCP_INC_STATS(sock_net(sk),

TCP_MIB_INERRS);

quoted

quoted

quoted

quoted

quoted

                NET_INC_STATS(sock_net(sk),

LINUX_MIB_TCPSYNCHALLENGE);

quoted

quoted

quoted

quoted

quoted

                tcp_send_challenge_ack(sk, skb);
-               goto discard;
+               tcp_drop(sk, skb, TCP_DROP_MASK(__LINE__,

TCP_VALIDATE_INCOMING));

quoted

quoted

quoted

quoted

I'd rather use a string. So that we can more easily identify

_why_ the

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

heavily modified kernels.
Having to download a git tree, or apply semi-private patches is

a no go.

quoted

quoted

quoted

quoted

If you really want to include __FILE__ and __LINE__, these both

can be

quoted

quoted

quoted

quoted

stringified and included in the report, with the help of

macros.

quoted

quoted

quoted

I agree the __LINE__ is pointless, but if this has a tracepoint
involved, then you can simply enable the stacktrace trigger to it

and

quoted

quoted

quoted

it will save a stack trace in the ring buffer for you.

   echo stacktrace >

/sys/kernel/tracing/events/tcp/tcp_drop/trigger

quoted

quoted

quoted

And when the event triggers it will record a stack trace. You can

also

quoted

quoted

quoted

even add a filter to do it only for specific reasons.

   echo 'stacktrace if reason == 1' >

/sys/kernel/tracing/events/tcp/tcp_drop/trigger

quoted

quoted

quoted

And it even works for flags:

   echo 'stacktrace if reason & 0xa' >

/sys/kernel/tracing/events/tcp/tcp_drop/trigger

quoted

quoted

quoted

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

You can't do string comparisons? The more string support Ftrace

has,

quoted

quoted

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.

Note, the output is still text, not numeric, as the __print_symbolic() macro will convert the numbers to text.

Or is there another issue you have with the enum?

-- Steve

+               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)
);

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.