Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
On Tue, 2024-03-26 at 18:43 +0800, Jason Xing wrote: > On Tue, Mar 26, 2024 at 6:29 PM Paolo Abeni wrote: > > > > On Tue, 2024-03-26 at 12:14 +0800, Jason Xing wrote: > > > On Mon, Mar 25, 2024 at 11:43 AM Jason Xing > > > wrote: > > > > > > > > From: Jason Xing > > > > > > > > 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
Re: [PATCH net-next v2 3/3] tcp: add location into reset trace process
On Mon, 2024-03-25 at 14:28 +0800, Jason Xing wrote: > From: Jason Xing > > In addition to knowing the 4-tuple of the flow which generates RST, > the reason why it does so is very important because we have some > cases where the RST should be sent and have no clue which one > exactly. > > Adding location of reset process can help us more, like what > trace_kfree_skb does. > > Signed-off-by: Jason Xing > --- > include/trace/events/tcp.h | 14 ++ > net/ipv4/tcp_ipv4.c| 2 +- > net/ipv4/tcp_output.c | 2 +- > net/ipv6/tcp_ipv6.c| 2 +- > 4 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h > index a13eb2147a02..8f6c1a07503c 100644 > --- a/include/trace/events/tcp.h > +++ b/include/trace/events/tcp.h > @@ -109,13 +109,17 @@ DEFINE_EVENT(tcp_event_sk_skb, tcp_retransmit_skb, > */ > TRACE_EVENT(tcp_send_reset, > > - TP_PROTO(const struct sock *sk, const struct sk_buff *skb), > + TP_PROTO( > + const struct sock *sk, > + const struct sk_buff *skb, > + void *location), Very minor nit: the above lines should be aligned with the open bracket. No need to repost just for this, but let's wait for Eric's feedback. Cheers, Paolo
Re: [PATCH net-next 0/3] trace: use TP_STORE_ADDRS macro
On Tue, 2024-03-26 at 12:14 +0800, Jason Xing wrote: > On Mon, Mar 25, 2024 at 11:43 AM Jason Xing wrote: > > > > From: Jason Xing > > > > 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
