Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751519AbeAGFjJ (ORCPT + 1 other); Sun, 7 Jan 2018 00:39:09 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:43298 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751025AbeAGFjH (ORCPT ); Sun, 7 Jan 2018 00:39:07 -0500 X-Google-Smtp-Source: ACJfBosnWrlaYdAJ3fi0xacxhLXukDjcYkm2baPfYiuQkCj2RFIdona7Cq3BrLGCoKDobSfCcpiEj/tj2DSSq/FpFZM= MIME-Version: 1.0 In-Reply-To: <544ED2FF-A44E-493D-B84E-46A58EEAB201@fb.com> References: <1515134569-23812-1-git-send-email-laoar.shao@gmail.com> <9A0D3ED9-7FFD-4E96-A42C-90871C2CC0C6@fb.com> <544ED2FF-A44E-493D-B84E-46A58EEAB201@fb.com> From: Yafang Shao Date: Sun, 7 Jan 2018 13:38:26 +0800 Message-ID: Subject: Re: [PATCH net-next] net: tracepoint: adding new tracepoint arguments in inet_sock_set_state To: Song Liu Cc: David Miller , "brendan.d.gregg@gmail.com" , "marcelo.leitner@gmail.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Sun, Jan 7, 2018 at 1:46 AM, Song Liu wrote: > >> On Jan 5, 2018, at 12:09 AM, Yafang Shao wrote: >> >> On Fri, Jan 5, 2018 at 3:21 PM, Song Liu wrote: >>> >>>> On Jan 4, 2018, at 10:42 PM, Yafang Shao wrote: >>>> >>>> sk->sk_protocol and sk->sk_family are exposed as tracepoint arguments. >>>> Then we can conveniently use these two arguments to do the filter. >>>> >>>> Suggested-by: Brendan Gregg >>>> Signed-off-by: Yafang Shao >>>> --- >>>> include/trace/events/sock.h | 24 ++++++++++++++++++------ >>>> net/ipv4/af_inet.c | 6 ++++-- >>>> 2 files changed, 22 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h >>>> index 3537c5f..c7df70f 100644 >>>> --- a/include/trace/events/sock.h >>>> +++ b/include/trace/events/sock.h >>>> @@ -11,7 +11,11 @@ >>>> #include >>>> #include >>>> >>>> -/* The protocol traced by sock_set_state */ >>>> +#define family_names \ >>>> + EM(AF_INET) \ >>>> + EMe(AF_INET6) >>>> + >>>> +/* The protocol traced by inet_sock_set_state */ >>>> #define inet_protocol_names \ >>>> EM(IPPROTO_TCP) \ >>>> EM(IPPROTO_DCCP) \ >>>> @@ -37,6 +41,7 @@ >>>> #define EM(a) TRACE_DEFINE_ENUM(a); >>>> #define EMe(a) TRACE_DEFINE_ENUM(a); >>>> >>>> +family_names >>>> inet_protocol_names >>>> tcp_state_names >>>> >>>> @@ -45,6 +50,9 @@ >>>> #define EM(a) { a, #a }, >>>> #define EMe(a) { a, #a } >>>> >>>> +#define show_family_name(val) \ >>>> + __print_symbolic(val, family_names) >>>> + >>>> #define show_inet_protocol_name(val) \ >>>> __print_symbolic(val, inet_protocol_names) >>>> >>>> @@ -108,9 +116,10 @@ >>>> >>>> TRACE_EVENT(inet_sock_set_state, >>>> >>>> - TP_PROTO(const struct sock *sk, const int oldstate, const int newstate), >>>> + TP_PROTO(const struct sock *sk, const int family, const int protocol, >>>> + const int oldstate, const int newstate), >>> >>> Are there cases we need protocol and/or family that is different to >>> sk->sk_protocol/sk_family? If not, I think we don't need to change the >>> TP_PROTO. >>> >>> Thanks, >>> Song >>> >> >> As of now, there're two sk_family, which are AF_INET and AF_INET6, and >> three sk_protocol which are IPPROTO_TCP, IPPROTO_SCTP and >> IPPROTO_DCCP. >> >> Thanks >> Yafang > > How about we do not change the TP_PROTO line? The patch will be like: > > @@ -118,6 +127,7 @@ > __field(int, newstate) > __field(__u16, sport) > __field(__u16, dport) > + __field(__u16, family) > __field(__u8, protocol) > __array(__u8, saddr, 4) > __array(__u8, daddr, 4) > @@ -133,8 +143,9 @@ > __entry->skaddr = sk; > __entry->oldstate = oldstate; > __entry->newstate = newstate; > + __entry->family = sk->sk_family; > __entry->protocol = sk->sk_protocol; > __entry->sport = ntohs(inet->inet_sport); > __entry->dport = ntohs(inet->inet_dport); > @@ xxxxx > - TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s", > + TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s", > + show_family_name(__entry->family), > Agreed. IMO that's a better solution, because we make littler change and could use the printk message to do the filter as well. I will submit a new patch. Thanks Yafang > >>>> >>>> - TP_ARGS(sk, oldstate, newstate), >>>> + TP_ARGS(sk, family, protocol, oldstate, newstate), >>>> >>>> TP_STRUCT__entry( >>>> __field(const void *, skaddr) >>>> @@ -118,6 +127,7 @@ >>>> __field(int, newstate) >>>> __field(__u16, sport) >>>> __field(__u16, dport) >>>> + __field(__u16, family) >>>> __field(__u8, protocol) >>>> __array(__u8, saddr, 4) >>>> __array(__u8, daddr, 4) >>>> @@ -133,8 +143,9 @@ >>>> __entry->skaddr = sk; >>>> __entry->oldstate = oldstate; >>>> __entry->newstate = newstate; >>>> + __entry->family = family; >>>> + __entry->protocol = protocol; >>>> >>>> - __entry->protocol = sk->sk_protocol; >>>> __entry->sport = ntohs(inet->inet_sport); >>>> __entry->dport = ntohs(inet->inet_dport); >>>> >>>> @@ -145,7 +156,7 @@ >>>> *p32 = inet->inet_daddr; >>>> >>>> #if IS_ENABLED(CONFIG_IPV6) >>>> - if (sk->sk_family == AF_INET6) { >>>> + if (family == AF_INET6) { >>>> pin6 = (struct in6_addr *)__entry->saddr_v6; >>>> *pin6 = sk->sk_v6_rcv_saddr; >>>> pin6 = (struct in6_addr *)__entry->daddr_v6; >>>> @@ -160,7 +171,8 @@ >>>> } >>>> ), >>>> >>>> - TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s", >>>> + TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s", >>>> + show_family_name(__entry->family), >>>> show_inet_protocol_name(__entry->protocol), >>>> __entry->sport, __entry->dport, >>>> __entry->saddr, __entry->daddr, >>>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c >>>> index bab98a4..1d52796 100644 >>>> --- a/net/ipv4/af_inet.c >>>> +++ b/net/ipv4/af_inet.c >>>> @@ -1223,14 +1223,16 @@ int inet_sk_rebuild_header(struct sock *sk) >>>> >>>> void inet_sk_set_state(struct sock *sk, int state) >>>> { >>>> - trace_inet_sock_set_state(sk, sk->sk_state, state); >>>> + trace_inet_sock_set_state(sk, sk->sk_family, sk->sk_protocol, >>>> + sk->sk_state, state); >>>> sk->sk_state = state; >>>> } >>>> EXPORT_SYMBOL(inet_sk_set_state); >>>> >>>> void inet_sk_state_store(struct sock *sk, int newstate) >>>> { >>>> - trace_inet_sock_set_state(sk, sk->sk_state, newstate); >>>> + trace_inet_sock_set_state(sk, sk->sk_family, sk->sk_protocol, >>>> + sk->sk_state, newstate); >>>> smp_store_release(&sk->sk_state, newstate); >>>> } >>>> >>>> -- >>>> 1.8.3.1 >