[PATCH net-next 0/2] add two missing addresses when using trace

[PATCH net-next 0/2] add two missing addresses when using trace

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

When I reviewed other people's patch [1], I noticed that similar thing
also happens in tcp_event_skb class and tcp_event_sk_skb class. They
don't print those two addrs of skb/sk which already exist.

They are probably forgotten by the original authors, so this time I
finish the work. Also, adding more trace about the socket/skb addr can
help us sometime even though the chance is minor.

I don't consider it is a bug, thus I chose to target net-next tree.

[1]
Link: https://lore.kernel.org/netdev/CAL+tcoAhvFhXdr1WQU8mv_6ZX5nOoNpbOLAB6=C+DB-qXQ11Ew@mail.gmail.com/

Jason Xing (2):
  tcp: add tracing of skb/skaddr in tcp_event_sk_skb class
  tcp: add tracing of skbaddr in tcp_event_skb class

 include/trace/events/tcp.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.37.3

[PATCH net-next 1/2] tcp: add tracing of skb/skaddr in tcp_event_sk_skb class

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Prio to this patch, the trace function doesn't print addresses
which might be forgotten. As we can see, it already fetches
those, use it directly and it will print like below:

...tcp_retransmit_skb: skbaddr=XXX skaddr=XXX family=AF_INET...

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

diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index 7b1ddffa3dfc..ac36067ae066 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -88,7 +88,8 @@ DECLARE_EVENT_CLASS(tcp_event_sk_skb,
 			      sk->sk_v6_rcv_saddr, sk->sk_v6_daddr);
 	),
 
-	TP_printk("family=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c state=%s",
+	TP_printk("skbaddr=%p skaddr=%p family=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c state=%s",
+		  __entry->skbaddr, __entry->skaddr,
 		  show_family_name(__entry->family),
 		  __entry->sport, __entry->dport, __entry->saddr, __entry->daddr,
 		  __entry->saddr_v6, __entry->daddr_v6,
-- 
2.37.3

Re: [PATCH net-next 1/2] tcp: add tracing of skb/skaddr in tcp_event_sk_skb class

[Eric Dumazet]

On Thu, Feb 29, 2024 at 6:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> Prio to this patch, the trace function doesn't print addresses
> which might be forgotten. As we can see, it already fetches
> those, use it directly and it will print like below:
>
> ...tcp_retransmit_skb: skbaddr=XXX skaddr=XXX family=AF_INET...

It was not forgotten, but a recommendation from Alexei

https://yhbt.net/lore/all/20171010173821.6djxyvrggvaivqec@ast-mbp.dhcp.thefacebook.com/

Re: [PATCH net-next 1/2] tcp: add tracing of skb/skaddr in tcp_event_sk_skb class

[Jason Xing]

On Mon, Mar 4, 2024 at 4:24 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Feb 29, 2024 at 6:10 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Prio to this patch, the trace function doesn't print addresses
> > which might be forgotten. As we can see, it already fetches
> > those, use it directly and it will print like below:
> >
> > ...tcp_retransmit_skb: skbaddr=XXX skaddr=XXX family=AF_INET...
>
> It was not forgotten, but a recommendation from Alexei
>
> https://yhbt.net/lore/all/20171010173821.6djxyvrggvaivqec@ast-mbp.dhcp.thefacebook.com/

Thanks, Eric.

Oh, good to see the old threads. I have to change the description. The
other reason why I chose to add address messages is we do have some
useful information when using trace ss another thread[1] does: it also
ships the trace with address printing.

[1]: https://lore.kernel.org/netdev/20240304034632.GA21357@didi-ThinkCentre-M920t-N000/

Thanks,
Jason

[PATCH net-next 2/2] tcp: add tracing of skbaddr in tcp_event_skb class

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Use the existing parameter and print it then. It will display
the address of skbaddr.

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

diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
index ac36067ae066..6ca3e0343666 100644
--- a/include/trace/events/tcp.h
+++ b/include/trace/events/tcp.h
@@ -362,7 +362,8 @@ DECLARE_EVENT_CLASS(tcp_event_skb,
 		TP_STORE_ADDR_PORTS_SKB(__entry, skb);
 	),
 
-	TP_printk("src=%pISpc dest=%pISpc", __entry->saddr, __entry->daddr)
+	TP_printk("skbaddr=%p src=%pISpc dest=%pISpc",
+		  __entry->skbaddr, __entry->saddr, __entry->daddr)
 );
 
 DEFINE_EVENT(tcp_event_skb, tcp_bad_csum,
-- 
2.37.3

Re: [PATCH net-next 0/2] add two missing addresses when using trace

[Jason Xing]

On Fri, Mar 1, 2024 at 1:10 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> When I reviewed other people's patch [1], I noticed that similar thing
> also happens in tcp_event_skb class and tcp_event_sk_skb class. They
> don't print those two addrs of skb/sk which already exist.
>
> They are probably forgotten by the original authors, so this time I
> finish the work. Also, adding more trace about the socket/skb addr can
> help us sometime even though the chance is minor.
>
> I don't consider it is a bug, thus I chose to target net-next tree.

Gentle ping...No rush. Just in case this simple patchset was lost for
some reason.

Thanks,
Jason

>
> [1]
> Link: https://lore.kernel.org/netdev/CAL+tcoAhvFhXdr1WQU8mv_6ZX5nOoNpbOLAB6=C+DB-qXQ11Ew@mail.gmail.com/
>
> Jason Xing (2):
>   tcp: add tracing of skb/skaddr in tcp_event_sk_skb class
>   tcp: add tracing of skbaddr in tcp_event_skb class
>
>  include/trace/events/tcp.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --
> 2.37.3
>

Re: [PATCH net-next 0/2] add two missing addresses when using trace

[Eric Dumazet]

On Mon, Mar 4, 2024 at 8:01 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Fri, Mar 1, 2024 at 1:10 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > When I reviewed other people's patch [1], I noticed that similar thing
> > also happens in tcp_event_skb class and tcp_event_sk_skb class. They
> > don't print those two addrs of skb/sk which already exist.
> >
> > They are probably forgotten by the original authors, so this time I
> > finish the work. Also, adding more trace about the socket/skb addr can
> > help us sometime even though the chance is minor.
> >
> > I don't consider it is a bug, thus I chose to target net-next tree.
>
> Gentle ping...No rush. Just in case this simple patchset was lost for
> some reason.

This was a conscious choice I think.

https://yhbt.net/lore/all/20171010173821.6djxyvrggvaivqec@ast-mbp.dhcp.thefacebook.com/