netdev - Re: [External] Re: [PATCH v4] sock: add tracepoint for send recv length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKY5gOC97NobXkhYv6d9ik=ks5ZEwVe=6H-VTwux=BwGQ@mail.gmail.com>
Date:   Mon, 9 Jan 2023 15:54:38 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     运辉崔 <cuiyunhui@...edance.com>
Cc:     rostedt@...dmis.org, mhiramat@...nel.org, davem@...emloft.net,
        kuba@...nel.org, pabeni@...hat.com, kuniyu@...zon.com,
        xiyou.wangcong@...il.com, duanxiongchun@...edance.com,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [External] Re: [PATCH v4] sock: add tracepoint for send recv length

On Mon, Jan 9, 2023 at 2:13 PM 运辉崔 <cuiyunhui@...edance.com> wrote:
>
> On Mon, Jan 9, 2023 at 5:56 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> >
> > Note: At least for CONFIG_RETPOLINE=y and gcc 12.2, compiler adds many
> > 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，right?
>
> #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 family,
>                                             __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 = INDIRECT_CALL_INET(sock->ops->sendmsg, inet6_sendmsg,
>                                      inet_sendmsg, sock, msg,
>                                      msg_data_left(msg));
>         BUG_ON(ret == -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