netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
@ 2024-03-10 12:14 Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h Jason Xing
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-10 12:14 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

Using the macro for other tracepoints use to be more concise.
No functional change.

Jason Xing (3):
  trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
  trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
  trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()

 include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
 include/trace/events/sock.h             | 35 ++++---------------------
 include/trace/events/tcp.h              | 29 --------------------
 3 files changed, 34 insertions(+), 59 deletions(-)

-- 
2.37.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net-next 1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
  2024-03-10 12:14 [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jason Xing
@ 2024-03-10 12:14 ` Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 2/3] trace: use TP_STORE_ADDRS() macro in inet_sk_error_report() Jason Xing
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-10 12:14 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

Put the macro into another standalone file for better extension.
Some tracepoints can use this common part in the future.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/trace/events/net_probe_common.h | 29 +++++++++++++++++++++++++
 include/trace/events/tcp.h              | 29 -------------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/trace/events/net_probe_common.h b/include/trace/events/net_probe_common.h
index 3930119cab08..b1f9a4d3ee13 100644
--- a/include/trace/events/net_probe_common.h
+++ b/include/trace/events/net_probe_common.h
@@ -41,4 +41,33 @@
 
 #endif
 
+#define TP_STORE_V4MAPPED(__entry, saddr, daddr)		\
+	do {							\
+		struct in6_addr *pin6;				\
+								\
+		pin6 = (struct in6_addr *)__entry->saddr_v6;	\
+		ipv6_addr_set_v4mapped(saddr, pin6);		\
+		pin6 = (struct in6_addr *)__entry->daddr_v6;	\
+		ipv6_addr_set_v4mapped(daddr, pin6);		\
+	} while (0)
+
+#if IS_ENABLED(CONFIG_IPV6)
+#define TP_STORE_ADDRS(__entry, saddr, daddr, saddr6, daddr6)		\
+	do {								\
+		if (sk->sk_family == AF_INET6) {			\
+			struct in6_addr *pin6;				\
+									\
+			pin6 = (struct in6_addr *)__entry->saddr_v6;	\
+			*pin6 = saddr6;					\
+			pin6 = (struct in6_addr *)__entry->daddr_v6;	\
+			*pin6 = daddr6;					\
+		} else {						\
+			TP_STORE_V4MAPPED(__entry, saddr, daddr);	\
+		}							\
+	} while (0)
+#else
+#define TP_STORE_ADDRS(__entry, saddr, daddr, saddr6, daddr6)	\
+	TP_STORE_V4MAPPED(__entry, saddr, daddr)
+#endif
+
 #endif
diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 699dafd204ea..3c08a0846c47 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -12,35 +12,6 @@
 #include <net/tcp.h>
 #include <linux/sock_diag.h>
 
-#define TP_STORE_V4MAPPED(__entry, saddr, daddr)		\
-	do {							\
-		struct in6_addr *pin6;				\
-								\
-		pin6 = (struct in6_addr *)__entry->saddr_v6;	\
-		ipv6_addr_set_v4mapped(saddr, pin6);		\
-		pin6 = (struct in6_addr *)__entry->daddr_v6;	\
-		ipv6_addr_set_v4mapped(daddr, pin6);		\
-	} while (0)
-
-#if IS_ENABLED(CONFIG_IPV6)
-#define TP_STORE_ADDRS(__entry, saddr, daddr, saddr6, daddr6)		\
-	do {								\
-		if (sk->sk_family == AF_INET6) {			\
-			struct in6_addr *pin6;				\
-									\
-			pin6 = (struct in6_addr *)__entry->saddr_v6;	\
-			*pin6 = saddr6;					\
-			pin6 = (struct in6_addr *)__entry->daddr_v6;	\
-			*pin6 = daddr6;					\
-		} else {						\
-			TP_STORE_V4MAPPED(__entry, saddr, daddr);	\
-		}							\
-	} while (0)
-#else
-#define TP_STORE_ADDRS(__entry, saddr, daddr, saddr6, daddr6)	\
-	TP_STORE_V4MAPPED(__entry, saddr, daddr)
-#endif
-
 /*
  * tcp event with arguments sk and skb
  *
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next 2/3] trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
  2024-03-10 12:14 [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h Jason Xing
@ 2024-03-10 12:14 ` Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 3/3] trace: use TP_STORE_ADDRS() macro in inet_sock_set_state() Jason Xing
  2024-03-11 23:21 ` [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jakub Kicinski
  3 siblings, 0 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-10 12:14 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

As the title said, use the macro directly like the patch[1] did
to avoid those duplications. No functional change.

[1]
commit 6a6b0b9914e7 ("tcp: Avoid preprocessor directives in tracepoint macro args")

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/trace/events/sock.h | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h
index fd206a6ab5b8..4397f7bfa406 100644
--- a/include/trace/events/sock.h
+++ b/include/trace/events/sock.h
@@ -10,6 +10,7 @@
 #include <linux/tracepoint.h>
 #include <linux/ipv6.h>
 #include <linux/tcp.h>
+#include <trace/events/net_probe_common.h>
 
 #define family_names			\
 		EM(AF_INET)				\
@@ -223,7 +224,6 @@ TRACE_EVENT(inet_sk_error_report,
 
 	TP_fast_assign(
 		const struct inet_sock *inet = inet_sk(sk);
-		struct in6_addr *pin6;
 		__be32 *p32;
 
 		__entry->error = sk->sk_err;
@@ -238,20 +238,8 @@ TRACE_EVENT(inet_sk_error_report,
 		p32 = (__be32 *) __entry->daddr;
 		*p32 =  inet->inet_daddr;
 
-#if IS_ENABLED(CONFIG_IPV6)
-		if (sk->sk_family == AF_INET6) {
-			pin6 = (struct in6_addr *)__entry->saddr_v6;
-			*pin6 = sk->sk_v6_rcv_saddr;
-			pin6 = (struct in6_addr *)__entry->daddr_v6;
-			*pin6 = sk->sk_v6_daddr;
-		} else
-#endif
-		{
-			pin6 = (struct in6_addr *)__entry->saddr_v6;
-			ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
-			pin6 = (struct in6_addr *)__entry->daddr_v6;
-			ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
-		}
+		TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
+			       sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
 	),
 
 	TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c error=%d",
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next 3/3] trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
  2024-03-10 12:14 [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h Jason Xing
  2024-03-10 12:14 ` [PATCH net-next 2/3] trace: use TP_STORE_ADDRS() macro in inet_sk_error_report() Jason Xing
@ 2024-03-10 12:14 ` Jason Xing
  2024-03-11 23:21 ` [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jakub Kicinski
  3 siblings, 0 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-10 12:14 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

As the title said, use the macro directly like the patch[1] did
to avoid those duplications. No functional change.

[1]
commit 6a6b0b9914e7 ("tcp: Avoid preprocessor directives in tracepoint macro args")

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/trace/events/sock.h | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h
index 4397f7bfa406..0d1c5ce4e6a6 100644
--- a/include/trace/events/sock.h
+++ b/include/trace/events/sock.h
@@ -160,7 +160,6 @@ TRACE_EVENT(inet_sock_set_state,
 
 	TP_fast_assign(
 		const struct inet_sock *inet = inet_sk(sk);
-		struct in6_addr *pin6;
 		__be32 *p32;
 
 		__entry->skaddr = sk;
@@ -178,20 +177,8 @@ TRACE_EVENT(inet_sock_set_state,
 		p32 = (__be32 *) __entry->daddr;
 		*p32 =  inet->inet_daddr;
 
-#if IS_ENABLED(CONFIG_IPV6)
-		if (sk->sk_family == AF_INET6) {
-			pin6 = (struct in6_addr *)__entry->saddr_v6;
-			*pin6 = sk->sk_v6_rcv_saddr;
-			pin6 = (struct in6_addr *)__entry->daddr_v6;
-			*pin6 = sk->sk_v6_daddr;
-		} else
-#endif
-		{
-			pin6 = (struct in6_addr *)__entry->saddr_v6;
-			ipv6_addr_set_v4mapped(inet->inet_saddr, pin6);
-			pin6 = (struct in6_addr *)__entry->daddr_v6;
-			ipv6_addr_set_v4mapped(inet->inet_daddr, pin6);
-		}
+		TP_STORE_ADDRS(__entry, inet->inet_saddr, inet->inet_daddr,
+			       sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
 	),
 
 	TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-10 12:14 [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jason Xing
                   ` (2 preceding siblings ...)
  2024-03-10 12:14 ` [PATCH net-next 3/3] trace: use TP_STORE_ADDRS() macro in inet_sock_set_state() Jason Xing
@ 2024-03-11 23:21 ` Jakub Kicinski
  3 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2024-03-11 23:21 UTC (permalink / raw)
  To: Jason Xing
  Cc: edumazet, mhiramat, mathieu.desnoyers, rostedt, pabeni, davem,
	netdev, linux-trace-kernel, Jason Xing

On Sun, 10 Mar 2024 20:14:03 +0800 Jason Xing wrote:
> Using the macro for other tracepoints use to be more concise.
> No functional change.

The merge window for 6.9 has started and we try not to apply patches 
to net-next during the merge window. Please repost in 2 weeks, once
Linus has tagged v6.9-rc1.
-- 
pw-bot: defer

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-26 13:18       ` Eric Dumazet
@ 2024-03-26 13:33         ` Jason Xing
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-26 13:33 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Paolo Abeni, mhiramat, mathieu.desnoyers, rostedt, kuba, davem,
	netdev, linux-trace-kernel, Jason Xing

On Tue, Mar 26, 2024 at 9:18 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Mar 26, 2024 at 11:44 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > Well, it's a pity that it seems that we are about to abandon this
> > method but it's not that friendly to the users who are unable to
> > deploy BPF...
>
> It is a pity these tracepoint patches are consuming a lot of reviewer
> time, just because
> some people 'can not deploy BPF'

Sure, not everyone can do this easily. The phenomenon still exists and
we cannot ignore it. Do you remember that about a month ago someone
submitted one patch introducing a new tracepoint and then I replied
to/asked you if it's necessary that we replace most of the tracepoints
with BPF? Now I realise and accept the fact...

I'll keep reviewing such patches and hope it can give you maintainers
a break. I don't mind taking some time to do it, after all it's not a
bad thing to help some people.

>
> Well, I came up with more ideas about how to improve the
> > trace function in recent days. The motivation of doing this is that I
> > encountered some issues which could be traced/diagnosed by using trace
> > effortlessly without writing some bpftrace codes again and again. The
> > status of trace seems not active but many people are still using it, I
> > believe.
>
> 'Writing bpftrace codes again and again' is not a good reason to add
> maintenance costs
> to linux networking stack.

I'm just saying :)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-26 10:43     ` Jason Xing
  2024-03-26 11:13       ` Paolo Abeni
@ 2024-03-26 13:18       ` Eric Dumazet
  2024-03-26 13:33         ` Jason Xing
  1 sibling, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2024-03-26 13:18 UTC (permalink / raw)
  To: Jason Xing
  Cc: Paolo Abeni, mhiramat, mathieu.desnoyers, rostedt, kuba, davem,
	netdev, linux-trace-kernel, Jason Xing

On Tue, Mar 26, 2024 at 11:44 AM Jason Xing <kerneljasonxing@gmail.com> wrote:

> Well, it's a pity that it seems that we are about to abandon this
> method but it's not that friendly to the users who are unable to
> deploy BPF...

It is a pity these tracepoint patches are consuming a lot of reviewer
time, just because
some people 'can not deploy BPF'

Well, I came up with more ideas about how to improve the
> trace function in recent days. The motivation of doing this is that I
> encountered some issues which could be traced/diagnosed by using trace
> effortlessly without writing some bpftrace codes again and again. The
> status of trace seems not active but many people are still using it, I
> believe.

'Writing bpftrace codes again and again' is not a good reason to add
maintenance costs
to linux networking stack.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-26 10:43     ` Jason Xing
@ 2024-03-26 11:13       ` Paolo Abeni
  2024-03-26 13:18       ` Eric Dumazet
  1 sibling, 0 replies; 13+ messages in thread
From: Paolo Abeni @ 2024-03-26 11:13 UTC (permalink / raw)
  To: Jason Xing
  Cc: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, davem,
	netdev, linux-trace-kernel, Jason Xing

On Tue, 2024-03-26 at 18:43 +0800, Jason Xing wrote:
> On Tue, Mar 26, 2024 at 6:29 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Tue, 2024-03-26 at 12:14 +0800, Jason Xing wrote:
> > > On Mon, Mar 25, 2024 at 11:43 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > > 
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > > 
> > > > Using the macro for other tracepoints use to be more concise.
> > > > No functional change.
> > > > 
> > > > Jason Xing (3):
> > > >   trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
> > > >   trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
> > > >   trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
> > > > 
> > > >  include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
> > > >  include/trace/events/sock.h             | 35 ++++---------------------
> > > 
> > > I just noticed that some trace files in include/trace directory (like
> > > net_probe_common.h, sock.h, skb.h, net.h, sock.h, udp.h, sctp.h,
> > > qdisc.h, neigh.h, napi.h, icmp.h, ...) are not owned by networking
> > > folks while some files (like tcp.h) have been maintained by specific
> > > maintainers/experts (like Eric) because they belong to one specific
> > > area. I wonder if we can get more networking guys involved in net
> > > tracing.
> > > 
> > > I'm not sure if 1) we can put those files into the "NETWORKING
> > > [GENERAL]" category, or 2) we can create a new category to include
> > > them all.
> > 
> > I think all the file you mentioned are not under networking because of
> > MAINTAINER file inaccuracy, and we could move there them accordingly.
> 
> Yes, they are not under the networking category currently. So how
> could we move them? The MAINTAINER file doesn't have all the specific
> categories which are suitable for each of the trace files.

I think there is no need to other categories: adding the explicit 'F:'
entries for such files in the NETWORKING [GENERAL] section should fit.

> > > I know people start using BPF to trace them all instead, but I can see
> > > some good advantages of those hooks implemented in the kernel, say:
> > > 1) help those machines which are not easy to use BPF tools.
> > > 2) insert the tracepoint in the middle of some functions which cannot
> > > be replaced by bpf kprobe.
> > > 3) if we have enough tracepoints, we can generate a timeline to
> > > know/detect which flow/skb spends unexpected time at which point.
> > > ...
> > > We can do many things in this area, I think :)
> > > 
> > > What do you think about this, Jakub, Paolo, Eric ?
> > 
> > I agree tracepoints are useful, but I think the general agreement is
> > that they are the 'old way', we should try to avoid their
> > proliferation.
> 
> Well, it's a pity that it seems that we are about to abandon this
> method but it's not that friendly to the users who are unable to
> deploy BPF... Well, I came up with more ideas about how to improve the
> trace function in recent days. The motivation of doing this is that I
> encountered some issues which could be traced/diagnosed by using trace
> effortlessly without writing some bpftrace codes again and again. The
> status of trace seems not active but many people are still using it, I
> believe.

I don't think we should abandon it completely. My understanding is that
we should thing carefully before adding new tracepoints, and generally
speaking, avoid adding 'too many' of them.

Cheers,

Paolo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-25  3:43 Jason Xing
  2024-03-26  4:14 ` Jason Xing
@ 2024-03-26 10:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-03-26 10:50 UTC (permalink / raw)
  To: Jason Xing
  Cc: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni,
	davem, netdev, linux-trace-kernel, kernelxing

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 25 Mar 2024 11:43:44 +0800 you wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Using the macro for other tracepoints use to be more concise.
> No functional change.
> 
> Jason Xing (3):
>   trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
>   trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
>   trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
    https://git.kernel.org/netdev/net-next/c/b3af9045b482
  - [net-next,2/3] trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
    https://git.kernel.org/netdev/net-next/c/a24c855a5ef2
  - [net-next,3/3] trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
    https://git.kernel.org/netdev/net-next/c/646700ce23f4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-26 10:29   ` Paolo Abeni
@ 2024-03-26 10:43     ` Jason Xing
  2024-03-26 11:13       ` Paolo Abeni
  2024-03-26 13:18       ` Eric Dumazet
  0 siblings, 2 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-26 10:43 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, davem,
	netdev, linux-trace-kernel, Jason Xing

On Tue, Mar 26, 2024 at 6:29 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2024-03-26 at 12:14 +0800, Jason Xing wrote:
> > On Mon, Mar 25, 2024 at 11:43 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Using the macro for other tracepoints use to be more concise.
> > > No functional change.
> > >
> > > Jason Xing (3):
> > >   trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
> > >   trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
> > >   trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
> > >
> > >  include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
> > >  include/trace/events/sock.h             | 35 ++++---------------------
> >
> > I just noticed that some trace files in include/trace directory (like
> > net_probe_common.h, sock.h, skb.h, net.h, sock.h, udp.h, sctp.h,
> > qdisc.h, neigh.h, napi.h, icmp.h, ...) are not owned by networking
> > folks while some files (like tcp.h) have been maintained by specific
> > maintainers/experts (like Eric) because they belong to one specific
> > area. I wonder if we can get more networking guys involved in net
> > tracing.
> >
> > I'm not sure if 1) we can put those files into the "NETWORKING
> > [GENERAL]" category, or 2) we can create a new category to include
> > them all.
>
> I think all the file you mentioned are not under networking because of
> MAINTAINER file inaccuracy, and we could move there them accordingly.

Yes, they are not under the networking category currently. So how
could we move them? The MAINTAINER file doesn't have all the specific
categories which are suitable for each of the trace files.

> >
> > I know people start using BPF to trace them all instead, but I can see
> > some good advantages of those hooks implemented in the kernel, say:
> > 1) help those machines which are not easy to use BPF tools.
> > 2) insert the tracepoint in the middle of some functions which cannot
> > be replaced by bpf kprobe.
> > 3) if we have enough tracepoints, we can generate a timeline to
> > know/detect which flow/skb spends unexpected time at which point.
> > ...
> > We can do many things in this area, I think :)
> >
> > What do you think about this, Jakub, Paolo, Eric ?
>
> I agree tracepoints are useful, but I think the general agreement is
> that they are the 'old way', we should try to avoid their
> proliferation.

Well, it's a pity that it seems that we are about to abandon this
method but it's not that friendly to the users who are unable to
deploy BPF... Well, I came up with more ideas about how to improve the
trace function in recent days. The motivation of doing this is that I
encountered some issues which could be traced/diagnosed by using trace
effortlessly without writing some bpftrace codes again and again. The
status of trace seems not active but many people are still using it, I
believe.

Thanks,
Jason

>
> Cheers,
>
> Paolo
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-26  4:14 ` Jason Xing
@ 2024-03-26 10:29   ` Paolo Abeni
  2024-03-26 10:43     ` Jason Xing
  0 siblings, 1 reply; 13+ messages in thread
From: Paolo Abeni @ 2024-03-26 10:29 UTC (permalink / raw)
  To: Jason Xing, edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, davem
  Cc: netdev, linux-trace-kernel, Jason Xing

On Tue, 2024-03-26 at 12:14 +0800, Jason Xing wrote:
> On Mon, Mar 25, 2024 at 11:43 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > 
> > From: Jason Xing <kernelxing@tencent.com>
> > 
> > Using the macro for other tracepoints use to be more concise.
> > No functional change.
> > 
> > Jason Xing (3):
> >   trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
> >   trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
> >   trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
> > 
> >  include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
> >  include/trace/events/sock.h             | 35 ++++---------------------
> 
> I just noticed that some trace files in include/trace directory (like
> net_probe_common.h, sock.h, skb.h, net.h, sock.h, udp.h, sctp.h,
> qdisc.h, neigh.h, napi.h, icmp.h, ...) are not owned by networking
> folks while some files (like tcp.h) have been maintained by specific
> maintainers/experts (like Eric) because they belong to one specific
> area. I wonder if we can get more networking guys involved in net
> tracing.
> 
> I'm not sure if 1) we can put those files into the "NETWORKING
> [GENERAL]" category, or 2) we can create a new category to include
> them all.

I think all the file you mentioned are not under networking because of
MAINTAINER file inaccuracy, and we could move there them accordingly.
> 
> I know people start using BPF to trace them all instead, but I can see
> some good advantages of those hooks implemented in the kernel, say:
> 1) help those machines which are not easy to use BPF tools.
> 2) insert the tracepoint in the middle of some functions which cannot
> be replaced by bpf kprobe.
> 3) if we have enough tracepoints, we can generate a timeline to
> know/detect which flow/skb spends unexpected time at which point.
> ...
> We can do many things in this area, I think :)
> 
> What do you think about this, Jakub, Paolo, Eric ?

I agree tracepoints are useful, but I think the general agreement is
that they are the 'old way', we should try to avoid their
proliferation. 

Cheers,

Paolo


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
  2024-03-25  3:43 Jason Xing
@ 2024-03-26  4:14 ` Jason Xing
  2024-03-26 10:29   ` Paolo Abeni
  2024-03-26 10:50 ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 13+ messages in thread
From: Jason Xing @ 2024-03-26  4:14 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, Jason Xing

On Mon, Mar 25, 2024 at 11:43 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Using the macro for other tracepoints use to be more concise.
> No functional change.
>
> Jason Xing (3):
>   trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
>   trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
>   trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()
>
>  include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
>  include/trace/events/sock.h             | 35 ++++---------------------

I just noticed that some trace files in include/trace directory (like
net_probe_common.h, sock.h, skb.h, net.h, sock.h, udp.h, sctp.h,
qdisc.h, neigh.h, napi.h, icmp.h, ...) are not owned by networking
folks while some files (like tcp.h) have been maintained by specific
maintainers/experts (like Eric) because they belong to one specific
area. I wonder if we can get more networking guys involved in net
tracing.

I'm not sure if 1) we can put those files into the "NETWORKING
[GENERAL]" category, or 2) we can create a new category to include
them all.

I know people start using BPF to trace them all instead, but I can see
some good advantages of those hooks implemented in the kernel, say:
1) help those machines which are not easy to use BPF tools.
2) insert the tracepoint in the middle of some functions which cannot
be replaced by bpf kprobe.
3) if we have enough tracepoints, we can generate a timeline to
know/detect which flow/skb spends unexpected time at which point.
...
We can do many things in this area, I think :)

What do you think about this, Jakub, Paolo, Eric ?

Thanks,
Jason

>  include/trace/events/tcp.h              | 29 --------------------
>  3 files changed, 34 insertions(+), 59 deletions(-)
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
@ 2024-03-25  3:43 Jason Xing
  2024-03-26  4:14 ` Jason Xing
  2024-03-26 10:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 13+ messages in thread
From: Jason Xing @ 2024-03-25  3:43 UTC (permalink / raw)
  To: edumazet, mhiramat, mathieu.desnoyers, rostedt, kuba, pabeni, davem
  Cc: netdev, linux-trace-kernel, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

Using the macro for other tracepoints use to be more concise.
No functional change.

Jason Xing (3):
  trace: move to TP_STORE_ADDRS related macro to net_probe_common.h
  trace: use TP_STORE_ADDRS() macro in inet_sk_error_report()
  trace: use TP_STORE_ADDRS() macro in inet_sock_set_state()

 include/trace/events/net_probe_common.h | 29 ++++++++++++++++++++
 include/trace/events/sock.h             | 35 ++++---------------------
 include/trace/events/tcp.h              | 29 --------------------
 3 files changed, 34 insertions(+), 59 deletions(-)

-- 
2.37.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-03-26 13:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-10 12:14 [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jason Xing
2024-03-10 12:14 ` [PATCH net-next 1/3] trace: move to TP_STORE_ADDRS related macro to net_probe_common.h Jason Xing
2024-03-10 12:14 ` [PATCH net-next 2/3] trace: use TP_STORE_ADDRS() macro in inet_sk_error_report() Jason Xing
2024-03-10 12:14 ` [PATCH net-next 3/3] trace: use TP_STORE_ADDRS() macro in inet_sock_set_state() Jason Xing
2024-03-11 23:21 ` [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro Jakub Kicinski
2024-03-25  3:43 Jason Xing
2024-03-26  4:14 ` Jason Xing
2024-03-26 10:29   ` Paolo Abeni
2024-03-26 10:43     ` Jason Xing
2024-03-26 11:13       ` Paolo Abeni
2024-03-26 13:18       ` Eric Dumazet
2024-03-26 13:33         ` Jason Xing
2024-03-26 10:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).