Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230108025545.338-1-cuiyunhui@bytedance.com> <CANn89i+W__5-jDUdM=_97jzQy9Wq+n9KBEuOGjUi=Fxe_ntqbg@mail.gmail.com>
 <CAEEQ3wnoKqN+uTmMmUDJ9pp+YVaLmKnv42RApzPbNOGg6CRmnA@mail.gmail.com>
In-Reply-To: <CAEEQ3wnoKqN+uTmMmUDJ9pp+YVaLmKnv42RApzPbNOGg6CRmnA@mail.gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 9 Jan 2023 15:54:38 +0100
Message-ID: <CANn89iKY5gOC97NobXkhYv6d9ik=ks5ZEwVe=6H-VTwux=BwGQ@mail.gmail.com>
Subject: Re: [External] Re: [PATCH v4] sock: add tracepoint for send recv length
To:     =?UTF-8?B?6L+Q6L6J5bSU?= <cuiyunhui@bytedance.com>
Cc:     rostedt@goodmis.org, mhiramat@kernel.org, davem@davemloft.net,
        kuba@kernel.org, pabeni@redhat.com, kuniyu@amazon.com,
        xiyou.wangcong@gmail.com, duanxiongchun@bytedance.com,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jan 9, 2023 at 2:13 PM =E8=BF=90=E8=BE=89=E5=B4=94 <cuiyunhui@byted=
ance.com> wrote:
>
> On Mon, Jan 9, 2023 at 5:56 PM Eric Dumazet <edumazet@google.com> wrote:
>
> >
> > Note: At least for CONFIG_RETPOLINE=3Dy and gcc 12.2, compiler adds man=
y
> > additional instructions (and additional memory reads),
> > even when the trace point is not enabled.
> >
> > Contrary to some belief, adding a tracepoint is not always 'free'.
> > tail calls for example are replaced with normal calls.
> >
>
>
> >         .popsection
> >
> > # 0 "" 2
> > #NO_APP
> > .L106:
> > # net/socket.c:1008: }
> >         movl    %ebx, %eax      # <retval>,
> >         popq    %rbx    #
> >         popq    %rbp    #
> >         popq    %r12    #
> >         ret
> > .L111:
> > # ./include/trace/events/sock.h:308: DEFINE_EVENT(sock_msg_length,
> > sock_recv_length,
> >
>
> Hi Eric,  Thanks for your reply,  In fact, it is because the
> definition of the tracepoint function is inline,
> Not just these two tracepoints=EF=BC=8Cright?
>
> #define __DECLARE_TRACE(name, proto, args, cond, data_proto)            \
>       ...
>       static inline void trace_##name(proto)
>
> Regarding the above issue, I plan to optimize it like this:
>
> static noinline void call_trace_sock_send_length(struct sock *sk, __u16 f=
amily,
>                                             __u16 protocol, int ret, int =
flags)
> {
>         trace_sock_send_length(sk, family, protocol, ret, 0);
> }
>
> static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *=
msg)
> {
>         int ret =3D INDIRECT_CALL_INET(sock->ops->sendmsg, inet6_sendmsg,
>                                      inet_sendmsg, sock, msg,
>                                      msg_data_left(msg));
>         BUG_ON(ret =3D=3D -EIOCBQUEUED);
>
>         if (trace_sock_send_length_enabled()) {

A barrier() is needed here, with the current state of affairs.

IMO, ftrace/x86 experts should take care of this generic issue ?


>                 call_trace_sock_send_length(sock->sk, sock->sk->sk_family=
,
>                                             sock->sk->sk_protocol, ret, 0=
);
>         }
>         return ret;
> }
>
> What do you think?
>
> Thanks,
> Yunhui