From: Masami Hiramatsu <mhiramat@kernel.org>
To: David Miller <davem@davemloft.net>
Cc: mingo@kernel.org, ian.mcdonald@jandi.co.nz, vyasevich@gmail.com,
stephen@networkplumber.org, rostedt@goodmis.org,
peterz@infradead.org, tglx@linutronix.de,
linux-kernel@vger.kernel.org, hpa@zytor.com,
gerrit@erg.abdn.ac.uk, nhorman@tuxdriver.com,
dccp@vger.kernel.org, netdev@vger.kernel.org,
linux-sctp@vger.kernel.org, sfr@canb.auug.org.au
Subject: Re: [PATCH net-next v5 1/6] net: tcp: Add trace events for TCP congestion window tracing
Date: Wed, 27 Dec 2017 14:43:20 +0900 [thread overview]
Message-ID: <20171227144320.c01ca1fcc00ec38ca7aa41e3@kernel.org> (raw)
In-Reply-To: <20171226.185155.2132957966134649827.davem@davemloft.net>
On Tue, 26 Dec 2017 18:51:55 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Date: Fri, 22 Dec 2017 11:05:33 +0900
>
> > This adds an event to trace TCP stat variables with
> > slightly intrusive trace-event. This uses ftrace/perf
> > event log buffer to trace those state, no needs to
> > prepare own ring-buffer, nor custom user apps.
> >
> > User can use ftrace to trace this event as below;
> >
> > # cd /sys/kernel/debug/tracing
> > # echo 1 > events/tcp/tcp_probe/enable
> > (run workloads)
> > # cat trace
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ...
> > + TP_fast_assign(
> > + const struct tcp_sock *tp = tcp_sk(sk);
> > + const struct inet_sock *inet = inet_sk(sk);
> > +
> > + memset(__entry->saddr, 0, sizeof(struct sockaddr_in6));
> > + memset(__entry->daddr, 0, sizeof(struct sockaddr_in6));
> > +
> > + if (sk->sk_family == AF_INET) {
> > + struct sockaddr_in *v4 = (void *)__entry->saddr;
> > +
> > + v4->sin_family = AF_INET;
> > + v4->sin_port = inet->inet_sport;
> > + v4->sin_addr.s_addr = inet->inet_saddr;
> > + v4 = (void *)__entry->daddr;
> > + v4->sin_family = AF_INET;
> > + v4->sin_port = inet->inet_dport;
> > + v4->sin_addr.s_addr = inet->inet_daddr;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + } else if (sk->sk_family == AF_INET6) {
>
> It looks like doing this ifdef test inside of a trace macro is very
> undesirable because it upsets sparse.
>
> Please see the following commit which just went into 'net'.
OK, that's helpful for me how to avoid it :)
I'll update the series .
Thank you,
>
> ====================
> commit 6a6b0b9914e73a8a54253dd5f6f5e5dd5e4a756c
> Author: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Date: Thu Dec 21 10:29:09 2017 -0800
>
> tcp: Avoid preprocessor directives in tracepoint macro args
>
> Using a preprocessor directive to check for CONFIG_IPV6 in the middle of
> a DECLARE_EVENT_CLASS macro's arg list causes sparse to report a series
> of errors:
>
> ./include/trace/events/tcp.h:68:1: error: directive in argument list
> ./include/trace/events/tcp.h:75:1: error: directive in argument list
> ./include/trace/events/tcp.h:144:1: error: directive in argument list
> ./include/trace/events/tcp.h:151:1: error: directive in argument list
> ./include/trace/events/tcp.h:216:1: error: directive in argument list
> ./include/trace/events/tcp.h:223:1: error: directive in argument list
> ./include/trace/events/tcp.h:274:1: error: directive in argument list
> ./include/trace/events/tcp.h:281:1: error: directive in argument list
>
> Once sparse finds an error, it stops printing warnings for the file it
> is checking. This masks any sparse warnings that would normally be
> reported for the core TCP code.
>
> Instead, handle the preprocessor conditionals in a couple of auxiliary
> macros. This also has the benefit of reducing duplicate code.
>
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index 07cccca..ab34c56 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -25,6 +25,35 @@
> tcp_state_name(TCP_CLOSING), \
> tcp_state_name(TCP_NEW_SYN_RECV))
>
> +#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
> *
> @@ -50,7 +79,6 @@ DECLARE_EVENT_CLASS(tcp_event_sk_skb,
>
> TP_fast_assign(
> struct inet_sock *inet = inet_sk(sk);
> - struct in6_addr *pin6;
> __be32 *p32;
>
> __entry->skbaddr = skb;
> @@ -65,20 +93,8 @@ DECLARE_EVENT_CLASS(tcp_event_sk_skb,
> 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("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c",
> @@ -127,7 +143,6 @@ DECLARE_EVENT_CLASS(tcp_event_sk,
>
> TP_fast_assign(
> struct inet_sock *inet = inet_sk(sk);
> - struct in6_addr *pin6;
> __be32 *p32;
>
> __entry->skaddr = sk;
> @@ -141,20 +156,8 @@ DECLARE_EVENT_CLASS(tcp_event_sk,
> 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("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c",
> @@ -197,7 +200,6 @@ TRACE_EVENT(tcp_set_state,
>
> TP_fast_assign(
> struct inet_sock *inet = inet_sk(sk);
> - struct in6_addr *pin6;
> __be32 *p32;
>
> __entry->skaddr = sk;
> @@ -213,20 +215,8 @@ TRACE_EVENT(tcp_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("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
> @@ -256,7 +246,6 @@ TRACE_EVENT(tcp_retransmit_synack,
>
> TP_fast_assign(
> struct inet_request_sock *ireq = inet_rsk(req);
> - struct in6_addr *pin6;
> __be32 *p32;
>
> __entry->skaddr = sk;
> @@ -271,20 +260,8 @@ TRACE_EVENT(tcp_retransmit_synack,
> p32 = (__be32 *) __entry->daddr;
> *p32 = ireq->ir_rmt_addr;
>
> -#if IS_ENABLED(CONFIG_IPV6)
> - if (sk->sk_family == AF_INET6) {
> - pin6 = (struct in6_addr *)__entry->saddr_v6;
> - *pin6 = ireq->ir_v6_loc_addr;
> - pin6 = (struct in6_addr *)__entry->daddr_v6;
> - *pin6 = ireq->ir_v6_rmt_addr;
> - } else
> -#endif
> - {
> - pin6 = (struct in6_addr *)__entry->saddr_v6;
> - ipv6_addr_set_v4mapped(ireq->ir_loc_addr, pin6);
> - pin6 = (struct in6_addr *)__entry->daddr_v6;
> - ipv6_addr_set_v4mapped(ireq->ir_rmt_addr, pin6);
> - }
> + TP_STORE_ADDRS(__entry, ireq->ir_loc_addr, ireq->ir_rmt_addr,
> + ireq->ir_v6_loc_addr, ireq->ir_v6_rmt_addr);
> ),
>
> TP_printk("sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c",
--
Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2017-12-27 5:43 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-22 2:05 [PATCH net-next v5 0/6] net: tcp: sctp: dccp: Replace jprobe usage with trace events Masami Hiramatsu
2017-12-22 2:05 ` [PATCH net-next v5 1/6] net: tcp: Add trace events for TCP congestion window tracing Masami Hiramatsu
2017-12-26 23:51 ` David Miller
2017-12-27 5:43 ` Masami Hiramatsu [this message]
2017-12-22 2:06 ` [PATCH net-next v5 2/6] net: tcp: Remove TCP probe module Masami Hiramatsu
2017-12-22 2:06 ` [PATCH net-next v5 3/6] net: sctp: Add SCTP ACK tracking trace event Masami Hiramatsu
2017-12-22 2:07 ` [PATCH net-next v5 4/6] net: sctp: Remove debug SCTP probe module Masami Hiramatsu
2017-12-22 2:07 ` [PATCH net-next v5 5/6] net: dccp: Add DCCP sendmsg trace event Masami Hiramatsu
2017-12-22 2:08 ` [PATCH net-next v5 6/6] net: dccp: Remove dccpprobe module Masami Hiramatsu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171227144320.c01ca1fcc00ec38ca7aa41e3@kernel.org \
--to=mhiramat@kernel.org \
--cc=davem@davemloft.net \
--cc=dccp@vger.kernel.org \
--cc=gerrit@erg.abdn.ac.uk \
--cc=hpa@zytor.com \
--cc=ian.mcdonald@jandi.co.nz \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=sfr@canb.auug.org.au \
--cc=stephen@networkplumber.org \
--cc=tglx@linutronix.de \
--cc=vyasevich@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).