Re: [PATCH net-next] trace: tcp: Add tracepoint for tcp_sendmsg()

[Breno Leitao <leitao@debian.org>]

Hello Eric,

On Mon, Feb 24, 2025 at 08:23:00PM +0100, Eric Dumazet wrote:
> On Mon, Feb 24, 2025 at 8:13 PM Yonghong Song <yhs@meta.com> wrote:
> >
> > > ________________________________________
> > >
> > > On Mon, Feb 24, 2025 at 7:24 PM Breno Leitao <leitao@debian.org> wrote:
> > >>
> > >> Add a lightweight tracepoint to monitor TCP sendmsg operations, enabling
> > >> the tracing of TCP messages being sent.
> > >>
> > >> Meta has been using BPF programs to monitor this function for years,
> > >> indicating significant interest in observing this important
> > >> functionality. Adding a proper tracepoint provides a stable API for all
> > >> users who need visibility into TCP message transmission.
> > >>
> > >> The implementation uses DECLARE_TRACE instead of TRACE_EVENT to avoid
> > >> creating unnecessary trace event infrastructure and tracefs exports,
> > >> keeping the implementation minimal while stabilizing the API.
> > >>
> > >> Given that this patch creates a rawtracepoint, you could hook into it
> > >> using regular tooling, like bpftrace, using regular rawtracepoint
> > >> infrastructure, such as:
> > >>
> > >>         rawtracepoint:tcp_sendmsg_tp {
> > >>                 ....
> > >>         }
> > >
> > > I would expect tcp_sendmsg() being stable enough ?
> > >
> > > kprobe:tcp_sendmsg {
> > > }
> >
> > In LTO mode, tcp_sendmsg could be inlined cross files. For example,
> >
> >   net/ipv4/tcp.c:
> >        int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >   net/ipv4/tcp_bpf.c:
> >        ...
> >       return tcp_sendmsg(sk, msg, size);
> >   net/ipv6/af_inet6.c:
> >        ...
> >        return INDIRECT_CALL_2(prot->sendmsg, tcp_sendmsg, udpv6_sendmsg, ...)
> >
> > And this does happen in our production environment.
> 
> And we do not have a way to make the kprobe work even if LTO decided
> to inline a function ?
> 
> This seems like a tracing or LTO issue, this could be addressed there
> in a generic way
> and avoid many other patches to work around this.

I understand that the {raw}tracepoint ensures the compiler cannot
interfere with these hook points. For everything else, we rely on the
hope that the compiler behaves favorably, which is far from ideal.