From: Menglong Dong <redacted>
As Eric reported, the 'reason' field is not presented when trace the
kfree_skb event by perf:
$ perf record -e skb:kfree_skb -a sleep 10
$ perf script
ip_defrag 14605 [021] 221.614303: skb:kfree_skb:
skbaddr=0xffff9d2851242700 protocol=34525 location=0xffffffffa39346b1
reason:
The cause seems to be passing kernel address directly to TP_printk(),
which is not right. As the enum 'skb_drop_reason' is not exported to
user space through TRACE_DEFINE_ENUM(), perf can't get the drop reason
string from the 'reason' field, which is a number.
Therefore, we introduce the macro DEFINE_DROP_REASON(), which is used
to define the trace enum by TRACE_DEFINE_ENUM(). With the help of
DEFINE_DROP_REASON(), now we can remove the auto-generate that we
introduced in the commit ec43908dd556
("net: skb: use auto-generation to convert skb drop reason to string"),
and define the string array 'drop_reasons'.
Hmmmm...now we come back to the situation that have to maintain drop
reasons in both enum skb_drop_reason and DEFINE_DROP_REASON. But they
are both in dropreason.h, which makes it easier.
After this commit, now the format of kfree_skb is like this:
$ cat /tracing/events/skb/kfree_skb/format
name: kfree_skb
ID: 1524
format:
field:unsigned short common_type; offset:0; size:2; signed:0;
field:unsigned char common_flags; offset:2; size:1; signed:0;
field:unsigned char common_preempt_count; offset:3; size:1; signed:0;
field:int common_pid; offset:4; size:4; signed:1;
field:void * skbaddr; offset:8; size:8; signed:0;
field:void * location; offset:16; size:8; signed:0;
field:unsigned short protocol; offset:24; size:2; signed:0;
field:enum skb_drop_reason reason; offset:28; size:4; signed:0;
print fmt: "skbaddr=%p protocol=%u location=%p reason: %s", REC->skbaddr, REC->protocol, REC->location, __print_symbolic(REC->reason, { 1, "NOT_SPECIFIED" }, { 2, "NO_SOCKET" } ......
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Menglong Dong <redacted>
---
v2:
- undef FN/FNe after use it (Jakub Kicinski)
---
include/net/dropreason.h | 67 ++++++++++++++++++++++++++++++++++++++
include/trace/events/skb.h | 15 ++++++++-
net/core/.gitignore | 1 -
net/core/Makefile | 22 +------------
net/core/skbuff.c | 6 +++-
5 files changed, 87 insertions(+), 24 deletions(-)
delete mode 100644 net/core/.gitignore
From: Eric Dumazet <edumazet@google.com> Date: 2022-09-02 15:52:26
On Fri, Sep 2, 2022 at 7:18 AM [off-list ref] wrote:
From: Menglong Dong <redacted>
As Eric reported, the 'reason' field is not presented when trace the
kfree_skb event by perf:
$ perf record -e skb:kfree_skb -a sleep 10
$ perf script
ip_defrag 14605 [021] 221.614303: skb:kfree_skb:
skbaddr=0xffff9d2851242700 protocol=34525 location=0xffffffffa39346b1
reason:
The cause seems to be passing kernel address directly to TP_printk(),
which is not right. As the enum 'skb_drop_reason' is not exported to
user space through TRACE_DEFINE_ENUM(), perf can't get the drop reason
string from the 'reason' field, which is a number.
Therefore, we introduce the macro DEFINE_DROP_REASON(), which is used
to define the trace enum by TRACE_DEFINE_ENUM(). With the help of
DEFINE_DROP_REASON(), now we can remove the auto-generate that we
introduced in the commit ec43908dd556
("net: skb: use auto-generation to convert skb drop reason to string"),
and define the string array 'drop_reasons'.
Hmmmm...now we come back to the situation that have to maintain drop
reasons in both enum skb_drop_reason and DEFINE_DROP_REASON. But they
are both in dropreason.h, which makes it easier.
After this commit, now the format of kfree_skb is like this:
$ cat /tracing/events/skb/kfree_skb/format
name: kfree_skb
ID: 1524
format:
field:unsigned short common_type; offset:0; size:2; signed:0;
field:unsigned char common_flags; offset:2; size:1; signed:0;
field:unsigned char common_preempt_count; offset:3; size:1; signed:0;
field:int common_pid; offset:4; size:4; signed:1;
field:void * skbaddr; offset:8; size:8; signed:0;
field:void * location; offset:16; size:8; signed:0;
field:unsigned short protocol; offset:24; size:2; signed:0;
field:enum skb_drop_reason reason; offset:28; size:4; signed:0;
print fmt: "skbaddr=%p protocol=%u location=%p reason: %s", REC->skbaddr, REC->protocol, REC->location, __print_symbolic(REC->reason, { 1, "NOT_SPECIFIED" }, { 2, "NO_SOCKET" } ......
Reported-by: Eric Dumazet <edumazet@google.com>
Note that I also provided the sha1 of the faulty patch.
You should add a corresponding Fixes: tag, to help both humans and bots.
This would also hint that this patch should target net tree, not net-next ?
Signed-off-by: Menglong Dong <redacted>
---
v2:
- undef FN/FNe after use it (Jakub Kicinski)
On Fri, Sep 2, 2022 at 11:43 PM Eric Dumazet [off-list ref] wrote:
On Fri, Sep 2, 2022 at 7:18 AM [off-list ref] wrote:
quoted
From: Menglong Dong <redacted>
As Eric reported, the 'reason' field is not presented when trace the
kfree_skb event by perf:
$ perf record -e skb:kfree_skb -a sleep 10
$ perf script
ip_defrag 14605 [021] 221.614303: skb:kfree_skb:
skbaddr=0xffff9d2851242700 protocol=34525 location=0xffffffffa39346b1
reason:
The cause seems to be passing kernel address directly to TP_printk(),
which is not right. As the enum 'skb_drop_reason' is not exported to
user space through TRACE_DEFINE_ENUM(), perf can't get the drop reason
string from the 'reason' field, which is a number.
Therefore, we introduce the macro DEFINE_DROP_REASON(), which is used
to define the trace enum by TRACE_DEFINE_ENUM(). With the help of
DEFINE_DROP_REASON(), now we can remove the auto-generate that we
introduced in the commit ec43908dd556
("net: skb: use auto-generation to convert skb drop reason to string"),
and define the string array 'drop_reasons'.
Hmmmm...now we come back to the situation that have to maintain drop
reasons in both enum skb_drop_reason and DEFINE_DROP_REASON. But they
are both in dropreason.h, which makes it easier.
After this commit, now the format of kfree_skb is like this:
$ cat /tracing/events/skb/kfree_skb/format
name: kfree_skb
ID: 1524
format:
field:unsigned short common_type; offset:0; size:2; signed:0;
field:unsigned char common_flags; offset:2; size:1; signed:0;
field:unsigned char common_preempt_count; offset:3; size:1; signed:0;
field:int common_pid; offset:4; size:4; signed:1;
field:void * skbaddr; offset:8; size:8; signed:0;
field:void * location; offset:16; size:8; signed:0;
field:unsigned short protocol; offset:24; size:2; signed:0;
field:enum skb_drop_reason reason; offset:28; size:4; signed:0;
print fmt: "skbaddr=%p protocol=%u location=%p reason: %s", REC->skbaddr, REC->protocol, REC->location, __print_symbolic(REC->reason, { 1, "NOT_SPECIFIED" }, { 2, "NO_SOCKET" } ......
Reported-by: Eric Dumazet <edumazet@google.com>
Note that I also provided the sha1 of the faulty patch.
You should add a corresponding Fixes: tag, to help both humans and bots.
Okay, I'll add the Fixes tag......and the Link tag.
This would also hint that this patch should target net tree, not net-next ?
It seems that the net tree is more suitable. I'll change it.
Thanks!
quoted
Signed-off-by: Menglong Dong <redacted>
---
v2:
- undef FN/FNe after use it (Jakub Kicinski)
I would love some feedback from Steven :)
Hi, Steven! Do you have any feedback? Yes, I add the
TRACE_DEFINE_ENUM() back! (Hmm...I deleted it before,
without knowing its function :/ )
Thanks!
Menglong Dong
From: Steven Rostedt <rostedt@goodmis.org> Date: 2022-09-05 14:40:33
On Fri, 2 Sep 2022 08:43:07 -0700
Eric Dumazet [off-list ref] wrote:
quoted
---
v2:
- undef FN/FNe after use it (Jakub Kicinski)
I would love some feedback from Steven :)
The undef should be fine. I usually do not, but that's more of a preference
than a rule.
As long as the undef is done after the C portion of where the macro is used:
+#undef FN
+#define FN(reason) TRACE_DEFINE_ENUM(SKB_DROP_REASON_##reason);
+DEFINE_DROP_REASON(FN, FN) <<<--- C portion
+#undef FN
+#undef FNe
+#define FN(reason) { SKB_DROP_REASON_##reason, #reason },
+#define FNe(reason) { SKB_DROP_REASON_##reason, #reason }
+
/*
* Tracepoint for free an sk_buff:
*/
From: Eric Dumazet <edumazet@google.com> Date: 2022-10-27 15:32:22
On Mon, Sep 5, 2022 at 7:37 AM Steven Rostedt [off-list ref] wrote:
quoted hunk
On Fri, 2 Sep 2022 08:43:07 -0700
Eric Dumazet [off-list ref] wrote:
quoted
quoted
---
v2:
- undef FN/FNe after use it (Jakub Kicinski)
I would love some feedback from Steven :)
The undef should be fine. I usually do not, but that's more of a preference
than a rule.
As long as the undef is done after the C portion of where the macro is used:
+#undef FN
+#define FN(reason) TRACE_DEFINE_ENUM(SKB_DROP_REASON_##reason);
+DEFINE_DROP_REASON(FN, FN) <<<--- C portion
+#undef FN
+#undef FNe
+#define FN(reason) { SKB_DROP_REASON_##reason, #reason },
+#define FNe(reason) { SKB_DROP_REASON_##reason, #reason }
+
/*
* Tracepoint for free an sk_buff:
*/