Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

+ arm ML and maintainers

On Wed, Sep 4, 2024 at 6:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> Hey,
>
> I just recently realized that we are still missing multi-kprobe
> support for ARM64, which depends on CONFIG_FPROBE. And CONFIG_FPROBE
> seems to require CONFIG_HAVE_RETHOOK, which, it turns out, is not
> implemented for ARM64.
>
> It took me a while to realize what's going on, as I roughly remembered
> (and confirmed through lore search) that Masami's original rethook
> patches had arm64-specific bits. Long story short:
>
> 0f8f8030038a Revert "arm64: rethook: Add arm64 rethook implementation"
> 83acdce68949 arm64: rethook: Add arm64 rethook implementation
>
> The patch was landed and then reverted. I found some discussion online
> and it seems like the plan was to land arch-specific bits shortly
> after bpf-next PR.
>
> But it seems like that never happened. Why?
>
> I see s390x, RISC-V, loongarch (I'm not even mentioning x86-64) all
> have CONFIG_HAVE_RETHOOK, even powerpc is getting one (see [0]), it
> seems. How come ARM64 is the one left out?
>
> Can anyone please provide some context? And if that's just an
> oversight, can we prioritize landing this for ARM64 ASAP?
>
>   [0] https://lore.kernel.org/bpf/20240830113131.7597-1-adubey@linux.ibm.com/
>

Masami, Steven,

Does Linus have to be in CC to get any reply here? Come on, it's been
almost a full week.

Maybe ARM64 folks have some context?... And hopefully desire to see
this through so that ARM64 doesn't stick out as a lesser-supported
platform as far as tracing goes compared to loongarch, s390x, and
powerpc (which just landed rethook support, see [2]).

Note that there was already an implementation (see [1]), but for some
reason it never made it.

  [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
  [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/

>
> -- Andrii

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Steven Rostedt]

On Tue, 10 Sep 2024 11:23:29 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> Masami, Steven,
> 
> Does Linus have to be in CC to get any reply here? Come on, it's been
> almost a full week.

And we are busy getting ready for Plumbers. Go ahead Cc Linus, I'm sure he
doesn't care, and you will likely just annoy him unless this is a
regression. 

> 
> Maybe ARM64 folks have some context?... And hopefully desire to see
> this through so that ARM64 doesn't stick out as a lesser-supported
> platform as far as tracing goes compared to loongarch, s390x, and
> powerpc (which just landed rethook support, see [2]).
> 
> Note that there was already an implementation (see [1]), but for some
> reason it never made it.
> 
>   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
>   [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/

Masami's fprobe work is near the top of my priority list, but there's been
bugs reported that always take precedence over features. There's been fires
to put out that has caused this to be delayed.

There! I replied.

-- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Steven Rostedt]

On Tue, 10 Sep 2024 11:23:29 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> Does Linus have to be in CC to get any reply here? Come on, it's been
> almost a full week.

Just FYI, an email like this does piss people off. You are getting upset
for waiting "almost a full week"? A full week is what we tell people to
wait if they don't get a response. And your email was directed to multiple
people. Then pointing out myself and Masami because we didn't respond? We
are not arm64 maintainers, and that email looked more directed at them.

Funny part is, I was just about to start reviewing Masami's fprobe patches
when I read this. Now I feel reluctant to. I'll do it anyway because they
are Masami's patches, but if they were yours, I would have pushed it off a
week or two with that attitude.

Again, just letting you know.

-- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Tue, Sep 10, 2024 at 11:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 10 Sep 2024 11:23:29 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > Does Linus have to be in CC to get any reply here? Come on, it's been
> > almost a full week.
>
> Just FYI, an email like this does piss people off. You are getting upset
> for waiting "almost a full week"? A full week is what we tell people to

A full week to get a response to a question? Yes, I find it way too
long. I didn't ask for some complicated code review, did I? I don't
know who "we" are and where "we" tell people, but I disagree that one
week is acceptable latency to coordinate stuff like this across
multiple subsystems.

I understand that life happens, and sometimes times are busier than
normal, but then a quick reply along the lines "sorry, busy, will get
back later" would be nice and completely normal, and I don't think
it's too much to ask.

And having said that, there were replies to other emails on the
mailing list from you and Masami, and even reviews for patches that
were posted way after my email. So I do believe that everyone is busy,
but I don't buy not having time to write a quick reply.

> wait if they don't get a response. And your email was directed to multiple
> people. Then pointing out myself and Masami because we didn't respond? We
> are not arm64 maintainers, and that email looked more directed at them.

"pointing out"? You and Masami are maintainers of linux-trace tree,
and rethook is part of that. Masami's original code was the one in
question and I did expect a rather quick reply from him. If not
Masami, then you would have a context as well. Who else should I be
asking?

If ARM64 folks somehow have more context, it wouldn't be that hard to
mention and redirect, instead of ghosting my email.

>
> Funny part is, I was just about to start reviewing Masami's fprobe patches
> when I read this. Now I feel reluctant to. I'll do it anyway because they
> are Masami's patches, but if they were yours, I would have pushed it off a
> week or two with that attitude.

(I'll ignore all the personal stuff)

You are probably talking about [0]. But I was asking about [1], i.e.,
adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
can I still get a meaningful answer as for why that wasn't landed and
what prevents it from landing right now before Masami's 20-patch
series lands?

  [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
  [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/

>
> Again, just letting you know.
>
> -- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Steven Rostedt]

On Tue, 10 Sep 2024 13:29:57 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Tue, Sep 10, 2024 at 11:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, 10 Sep 2024 11:23:29 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >  
> > > Does Linus have to be in CC to get any reply here? Come on, it's been
> > > almost a full week.  
> >
> > Just FYI, an email like this does piss people off. You are getting upset
> > for waiting "almost a full week"? A full week is what we tell people to  
> 
> A full week to get a response to a question? Yes, I find it way too
> long. I didn't ask for some complicated code review, did I? I don't
> know who "we" are and where "we" tell people, but I disagree that one
> week is acceptable latency to coordinate stuff like this across
> multiple subsystems.

Why do I have to answer to you? Once I saw the "ARM64" in the subject, it
immediately went down in priority and honesty, I didn't even read it as I'm
not the ARM64 maintainer. I did skim it to see if my name was mentioned as
I usually try to do with emails, but when it wasn't I ignored it.

> 
> "pointing out"? You and Masami are maintainers of linux-trace tree,
> and rethook is part of that. Masami's original code was the one in

Yes, but I don't touch arm code. Masami sometimes does (as is the case
here), but it is when we work with the arm maintainers.

> question and I did expect a rather quick reply from him. If not
> Masami, then you would have a context as well. Who else should I be
> asking?

The arm64 maintainers as they are the ones that maintain that code.

> 
> If ARM64 folks somehow have more context, it wouldn't be that hard to
> mention and redirect, instead of ghosting my email.

You should know they have more context because they are the actual
maintainers. I shouldn't have to point that out to you.

 $ wget -O /tmp/t.patch https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/raw
 $ ./scripts/get_maintainer.pl t.patch
Catalin Marinas <catalin.marinas@arm.com> (maintainer:ARM64 PORT (AARCH64 ARCHITECTURE),commit_signer:2/6=33%)
Will Deacon <will@kernel.org> (maintainer:ARM64 PORT (AARCH64 ARCHITECTURE),commit_signer:5/6=83%)
Puranjay Mohan <puranjay@kernel.org> (commit_signer:5/6=83%,authored:3/6=50%,added_lines:30/255=12%)
Mark Rutland <mark.rutland@arm.com> (commit_signer:4/6=67%,authored:2/6=33%,added_lines:105/255=41%,removed_lines:47/49=96%)
"Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> (commit_signer:2/6=33%)
chenqiwu <qiwuchen55@gmail.com> (authored:1/6=17%,added_lines:120/255=47%)
linux-arm-kernel@lists.infradead.org (moderated list:ARM64 PORT (AARCH64 ARCHITECTURE))
linux-kernel@vger.kernel.org (open list)
bpf@vger.kernel.org (open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_))

Neither my name nor Masami's shows up.

> 
> >
> > Funny part is, I was just about to start reviewing Masami's fprobe patches
> > when I read this. Now I feel reluctant to. I'll do it anyway because they
> > are Masami's patches, but if they were yours, I would have pushed it off a
> > week or two with that attitude.  
> 
> (I'll ignore all the personal stuff)

Maybe you shouldn't ignore it. If you think you can get answers by jumping
immediately to "I'm going to tell on you to Linus", you may want to rethink
your approach. A simple "Hey Steve and Masami, what's going on?" would be
the "human" thing to do. Especially since you appear to be mad at us for
not replying to an email about code we do not maintain.

Sorry, but you're not my boss, I don't have to reply to any of your emails.

> 
> You are probably talking about [0]. But I was asking about [1], i.e.,
> adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> can I still get a meaningful answer as for why that wasn't landed and
> what prevents it from landing right now before Masami's 20-patch
> series lands?
> 
>   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
>   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> 
> >
> > Again, just letting you know.

Because [1] isn't something I maintain. So I ignored it.

 arch/arm64/Kconfig                            |    1 
 arch/arm64/include/asm/stacktrace.h           |    2 -
 arch/arm64/kernel/probes/Makefile             |    1 
 arch/arm64/kernel/probes/rethook.c            |   25 +++++++
 arch/arm64/kernel/probes/rethook_trampoline.S |   87 +++++++++++++++++++++++++
 arch/arm64/kernel/stacktrace.c                |    7 ++

None of that would go through my tree unless an arm64 maintainer asked.

In fact, I need a bunch of acks from all maintainers of the architectures
that are touched by [0] before I can pull it in. Which means it will likely
not make this merge window.

-- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Tue, Sep 10, 2024 at 3:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 10 Sep 2024 13:29:57 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Tue, Sep 10, 2024 at 11:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Tue, 10 Sep 2024 11:23:29 -0700
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >
> > > > Does Linus have to be in CC to get any reply here? Come on, it's been
> > > > almost a full week.
> > >
> > > Just FYI, an email like this does piss people off. You are getting upset
> > > for waiting "almost a full week"? A full week is what we tell people to
> >
> > A full week to get a response to a question? Yes, I find it way too
> > long. I didn't ask for some complicated code review, did I? I don't
> > know who "we" are and where "we" tell people, but I disagree that one
> > week is acceptable latency to coordinate stuff like this across
> > multiple subsystems.
>
> Why do I have to answer to you? Once I saw the "ARM64" in the subject, it
> immediately went down in priority and honesty, I didn't even read it as I'm
> not the ARM64 maintainer. I did skim it to see if my name was mentioned as
> I usually try to do with emails, but when it wasn't I ignored it.

So, in the end, it wasn't "And we are busy getting ready for
Plumbers.", but rather you didn't find the right keywords in my email,
right? "Masami" and "Steven" would be the right keywords, but
"CONFIG_FPROBE" and "CONFIG_RETHOOK" aren't. Good to know.

>
> >
> > "pointing out"? You and Masami are maintainers of linux-trace tree,
> > and rethook is part of that. Masami's original code was the one in
>
> Yes, but I don't touch arm code. Masami sometimes does (as is the case
> here), but it is when we work with the arm maintainers.

And? Did I ask you to write that code? Or review that code? Or did I
ask the context on why a portion of the patch set didn't end up
upstream, while the rest did. The patch set submitted by Masami and
signed off by and tested by you. Was it too much to expect that either
you or Masami will have a quick answer? I'm sorry, I didn't know you
don't really read emails addressed *directly* to you in email's To:,
my bad assuming as much.

>
> > question and I did expect a rather quick reply from him. If not
> > Masami, then you would have a context as well. Who else should I be
> > asking?
>
> The arm64 maintainers as they are the ones that maintain that code.

Even if I misrouted the question (which I still don't believe I did),
is it above you to point it out and CC the right people?

>
> >
> > If ARM64 folks somehow have more context, it wouldn't be that hard to
> > mention and redirect, instead of ghosting my email.
>
> You should know they have more context because they are the actual
> maintainers. I shouldn't have to point that out to you.

Maybe they do, maybe they don't. I'm relying and using
kprobes/kretprobes, and I still don't have a clear understanding of
all the nuances and differences of k[ret]probes, rethook, fprobe, and
ftrace, and what works with what. Call me dumb. I don't expect ARM64
maintainers to know these nuances. They are experts in
ARM64-specifics, not in a tracing layer, I presume.

>
>  $ wget -O /tmp/t.patch https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/raw
>  $ ./scripts/get_maintainer.pl t.patch
> Catalin Marinas <catalin.marinas@arm.com> (maintainer:ARM64 PORT (AARCH64 ARCHITECTURE),commit_signer:2/6=33%)
> Will Deacon <will@kernel.org> (maintainer:ARM64 PORT (AARCH64 ARCHITECTURE),commit_signer:5/6=83%)
> Puranjay Mohan <puranjay@kernel.org> (commit_signer:5/6=83%,authored:3/6=50%,added_lines:30/255=12%)
> Mark Rutland <mark.rutland@arm.com> (commit_signer:4/6=67%,authored:2/6=33%,added_lines:105/255=41%,removed_lines:47/49=96%)
> "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> (commit_signer:2/6=33%)
> chenqiwu <qiwuchen55@gmail.com> (authored:1/6=17%,added_lines:120/255=47%)
> linux-arm-kernel@lists.infradead.org (moderated list:ARM64 PORT (AARCH64 ARCHITECTURE))
> linux-kernel@vger.kernel.org (open list)
> bpf@vger.kernel.org (open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_))
>
> Neither my name nor Masami's shows up.

$ vim wget -O /tmp/t.patch
https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/raw
$ grep -E 'Masami|Steven' /tmp/t.patch
From:   Masami Hiramatsu <mhiramat@kernel.org>
        Masami Hiramatsu <mhiramat@kernel.org>, netdev@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>,
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Furthermore,

$ git grep 'config RETHOOK'
kernel/trace/Kconfig:config RETHOOK

$ scripts/get_maintainer.pl kernel/trace/Kconfig
Steven Rostedt <rostedt@goodmis.org> (maintainer:TRACING)
Masami Hiramatsu <mhiramat@kernel.org> (maintainer:TRACING)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> (reviewer:TRACING)
linux-kernel@vger.kernel.org (open list:TRACING)
linux-trace-kernel@vger.kernel.org (open list:TRACING)

You can define your responsibilities as narrow as you'd like. I was
asking a question about the RETHOOK patchset/feature overall and why a
portion of the original patch set is missing, in particular.

>
> >
> > >
> > > Funny part is, I was just about to start reviewing Masami's fprobe patches
> > > when I read this. Now I feel reluctant to. I'll do it anyway because they
> > > are Masami's patches, but if they were yours, I would have pushed it off a
> > > week or two with that attitude.
> >
> > (I'll ignore all the personal stuff)
>
> Maybe you shouldn't ignore it. If you think you can get answers by jumping
> immediately to "I'm going to tell on you to Linus", you may want to rethink

No I don't, and I'd hate to have to do that. Which is why I didn't CC
Linus. And I get that stuff slips through sometimes, as I said. But I
don't get your absolutely overblown reaction to a question born out of
frustration of being ignored.

> your approach. A simple "Hey Steve and Masami, what's going on?" would be
> the "human" thing to do. Especially since you appear to be mad at us for

Don't project, Steven. I'm not mad, though definitely frustrated by a
very unresponsive ML and its maintainers.

I tried a "hey Masami" approach in [0], and it didn't help much, unfortunately.

And it's not the first time I'm ghosted on this mailing list. Would
you say 4.5 months not getting any reply to [1] is long enough?
Though, let me guess, it's x86-specific and you don't have anything to
do with this, right? Going forward I'll consult get_maintainer.pl
every time to check if you are *NOT* responsible for something, my
bad. I didn't live by get_maintainer.pl up until now.

  [0] https://lore.kernel.org/bpf/CAEf4BzbbVRGROtRn8PM4h1493avHMggz1kSDDJcaNZ1USO_eVw@mail.gmail.com
  [1] https://lore.kernel.org/linux-trace-kernel/20240425000211.708557-1-andrii@kernel.org/

> not replying to an email about code we do not maintain.
>
> Sorry, but you're not my boss, I don't have to reply to any of your emails.

I didn't say I am, not sure where you got that from. But I did expect
a bit more ownership from you as a linux-trace tree maintainer. I'm
sorry.

>
> >
> > You are probably talking about [0]. But I was asking about [1], i.e.,
> > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> > can I still get a meaningful answer as for why that wasn't landed and
> > what prevents it from landing right now before Masami's 20-patch
> > series lands?
> >
> >   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
> >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> >
> > >
> > > Again, just letting you know.
>
> Because [1] isn't something I maintain. So I ignored it.

Yes, you are doing a great job at ignoring stuff. That I understood
very well, thank you.

>
>  arch/arm64/Kconfig                            |    1
>  arch/arm64/include/asm/stacktrace.h           |    2 -
>  arch/arm64/kernel/probes/Makefile             |    1
>  arch/arm64/kernel/probes/rethook.c            |   25 +++++++
>  arch/arm64/kernel/probes/rethook_trampoline.S |   87 +++++++++++++++++++++++++
>  arch/arm64/kernel/stacktrace.c                |    7 ++
>
> None of that would go through my tree unless an arm64 maintainer asked.
>
> In fact, I need a bunch of acks from all maintainers of the architectures
> that are touched by [0] before I can pull it in. Which means it will likely
> not make this merge window.
>
> -- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Steven Rostedt]

On Tue, 10 Sep 2024 17:27:06 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
\> >
> > Why do I have to answer to you? Once I saw the "ARM64" in the subject, it
> > immediately went down in priority and honesty, I didn't even read it as I'm
> > not the ARM64 maintainer. I did skim it to see if my name was mentioned as
> > I usually try to do with emails, but when it wasn't I ignored it.  
> 
> So, in the end, it wasn't "And we are busy getting ready for
> Plumbers.", but rather you didn't find the right keywords in my email,
> right? "Masami" and "Steven" would be the right keywords, but
> "CONFIG_FPROBE" and "CONFIG_RETHOOK" aren't. Good to know.

Yep. I'm Cc'd on approximately 300 emails a day. I don't have time to read
them all.


> > >  
> > > >
> > > > Funny part is, I was just about to start reviewing Masami's fprobe patches
> > > > when I read this. Now I feel reluctant to. I'll do it anyway because they
> > > > are Masami's patches, but if they were yours, I would have pushed it off a
> > > > week or two with that attitude.  
> > >
> > > (I'll ignore all the personal stuff)  
> >
> > Maybe you shouldn't ignore it. If you think you can get answers by jumping
> > immediately to "I'm going to tell on you to Linus", you may want to rethink  
> 
> No I don't, and I'd hate to have to do that. Which is why I didn't CC
> Linus. And I get that stuff slips through sometimes, as I said. But I
> don't get your absolutely overblown reaction to a question born out of
> frustration of being ignored.

My reaction was due to the:

> Does Linus have to be in CC to get any reply here? Come on, it's been
> almost a full week.

Which sounds more of a threat. There's a lot of email I miss. Especially
after I come back from vacation, which BTW I did in this case. I came back
on September 3rd, your email was dated September 4th, I had several
thousand unread emails. I don't have time to read them all, so yeah, I do a
lot of skimming, looking for the meat.

You went from sending an email to threatening Cc'ing Linus because I didn't
jump to reply to you. It took me three full days just to get through my
backlog from vacation, and then I have to deal with the emails coming in
while I'm digging through my backlog.

What pissed me off is that comment makes you sound like you think your
email is more important than all my other email. I could have simply not
told you this, and left out sending that reply, but I figured I'd let you
know what perception you gave to me.

> 
> > your approach. A simple "Hey Steve and Masami, what's going on?" would be
> > the "human" thing to do. Especially since you appear to be mad at us for  
> 
> Don't project, Steven. I'm not mad, though definitely frustrated by a
> very unresponsive ML and its maintainers.
> 
> I tried a "hey Masami" approach in [0], and it didn't help much, unfortunately.
> 
> And it's not the first time I'm ghosted on this mailing list. Would
> you say 4.5 months not getting any reply to [1] is long enough?

I'm "ghosted" all the time with other maintainers. When I ask a question or
something, I will wait a while and send a friendly "ping". It's common
practice.

> Though, let me guess, it's x86-specific and you don't have anything to
> do with this, right? Going forward I'll consult get_maintainer.pl
> every time to check if you are *NOT* responsible for something, my
> bad. I didn't live by get_maintainer.pl up until now.

I'm not saying you had to do that. But you should have done that before
threatening to go over my head.

> 
>   [0] https://lore.kernel.org/bpf/CAEf4BzbbVRGROtRn8PM4h1493avHMggz1kSDDJcaNZ1USO_eVw@mail.gmail.com
>   [1] https://lore.kernel.org/linux-trace-kernel/20240425000211.708557-1-andrii@kernel.org/
> 
> > not replying to an email about code we do not maintain.
> >
> > Sorry, but you're not my boss, I don't have to reply to any of your emails.  
> 
> I didn't say I am, not sure where you got that from. But I did expect
> a bit more ownership from you as a linux-trace tree maintainer. I'm
> sorry.
> 
> >  
> > >
> > > You are probably talking about [0]. But I was asking about [1], i.e.,
> > > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> > > can I still get a meaningful answer as for why that wasn't landed and
> > > what prevents it from landing right now before Masami's 20-patch
> > > series lands?
> > >
> > >   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
> > >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > >  
> > > >
> > > > Again, just letting you know.  
> >
> > Because [1] isn't something I maintain. So I ignored it.  
> 
> Yes, you are doing a great job at ignoring stuff. That I understood
> very well, thank you.

Yes, it's a balancing act of trying to get stuff done. Every time I focus
on one thing I tend to have to ignore something else.

-- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Steven Rostedt]

On Tue, 10 Sep 2024 21:27:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Does Linus have to be in CC to get any reply here? Come on, it's been
> > almost a full week.  

Moral of the story is, if you had left out the above sentences, I would have
updated you nicely and this would not have been an issue.

-- Steve

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Masami Hiramatsu]

On Tue, 10 Sep 2024 13:29:57 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> You are probably talking about [0]. But I was asking about [1], i.e.,
> adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> can I still get a meaningful answer as for why that wasn't landed and
> what prevents it from landing right now before Masami's 20-patch
> series lands?

As I replied to your last email, Mark discovered that [1] is incorrect.
 From the bpf perspective, it may be fine that struct pt_regs is missing
 some architecture-specific registers, but from an API perspective,
 it is a problem.

Actually kretprobes on arm64 still does not do it correctly, but I also
know most of users does not care. So currently I keep it as it is. But
after fixing this issue on fprobe. I would like to update kretprobe so
that it will use sw-breakpoint to handle it. It will increase the overhead
of kretprobe, but it should be replaced by fprobe at that moment.

Thank you,

> 
>   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
>   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> 
> >
> > Again, just letting you know.
> >
> > -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Tue, Sep 10, 2024 at 5:39 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 10 Sep 2024 13:29:57 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > You are probably talking about [0]. But I was asking about [1], i.e.,
> > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> > can I still get a meaningful answer as for why that wasn't landed and
> > what prevents it from landing right now before Masami's 20-patch
> > series lands?
>
> As I replied to your last email, Mark discovered that [1] is incorrect.
>  From the bpf perspective, it may be fine that struct pt_regs is missing
>  some architecture-specific registers, but from an API perspective,
>  it is a problem.
>
> Actually kretprobes on arm64 still does not do it correctly, but I also
> know most of users does not care. So currently I keep it as it is. But
> after fixing this issue on fprobe. I would like to update kretprobe so
> that it will use sw-breakpoint to handle it. It will increase the overhead
> of kretprobe, but it should be replaced by fprobe at that moment.

Ok, given kretprobes already have this issue, can we add this support
for BPF multi-kprobe/kretprobe only? We can have an extra Kconfig
option or whatever necessary. It's sad that we don't have entire
feature just because a few registers can't be set (and I bet no BPF
users ever reads those registers from pt_regs). It's not the first,
nor last case where pt_regs isn't complete (e.g., tracepoints set only
a few fields in pt_regs, the rest are zero; and that's fine).

>
> Thank you,
>
> >
> >   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
> >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> >
> > >
> > > Again, just letting you know.
> > >
> > > -- Steve
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Masami Hiramatsu]

On Tue, 10 Sep 2024 17:44:11 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Tue, Sep 10, 2024 at 5:39 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 10 Sep 2024 13:29:57 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > You are probably talking about [0]. But I was asking about [1], i.e.,
> > > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> > > can I still get a meaningful answer as for why that wasn't landed and
> > > what prevents it from landing right now before Masami's 20-patch
> > > series lands?
> >
> > As I replied to your last email, Mark discovered that [1] is incorrect.
> >  From the bpf perspective, it may be fine that struct pt_regs is missing
> >  some architecture-specific registers, but from an API perspective,
> >  it is a problem.
> >
> > Actually kretprobes on arm64 still does not do it correctly, but I also
> > know most of users does not care. So currently I keep it as it is. But
> > after fixing this issue on fprobe. I would like to update kretprobe so
> > that it will use sw-breakpoint to handle it. It will increase the overhead
> > of kretprobe, but it should be replaced by fprobe at that moment.
> 
> Ok, given kretprobes already have this issue, can we add this support
> for BPF multi-kprobe/kretprobe only? We can have an extra Kconfig
> option or whatever necessary. It's sad that we don't have entire
> feature just because a few registers can't be set (and I bet no BPF
> users ever reads those registers from pt_regs). It's not the first,
> nor last case where pt_regs isn't complete (e.g., tracepoints set only
> a few fields in pt_regs, the rest are zero; and that's fine).

pt_regs things are asked by PeterZ. It is not recommended to use pt_regs
if it is not actual pt_regs because user expects it works as full pt_regs.
So I and Steve decided to use ftrace_regs for this faked registers.
(I think tracepoint should also use ftrace_regs or another one.)

Anyway, we're almost at a goal we can all agree on. I think we would better
push fprobe on fgraph series instead of such ad-hoc change.

Thank you,

> 
> >
> > Thank you,
> >
> > >
> > >   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
> > >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > >
> > > >
> > > > Again, just letting you know.
> > > >
> > > > -- Steve
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Wed, Sep 11, 2024 at 8:26 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 10 Sep 2024 17:44:11 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Tue, Sep 10, 2024 at 5:39 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > On Tue, 10 Sep 2024 13:29:57 -0700
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >
> > > > You are probably talking about [0]. But I was asking about [1], i.e.,
> > > > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above,
> > > > can I still get a meaningful answer as for why that wasn't landed and
> > > > what prevents it from landing right now before Masami's 20-patch
> > > > series lands?
> > >
> > > As I replied to your last email, Mark discovered that [1] is incorrect.
> > >  From the bpf perspective, it may be fine that struct pt_regs is missing
> > >  some architecture-specific registers, but from an API perspective,
> > >  it is a problem.
> > >
> > > Actually kretprobes on arm64 still does not do it correctly, but I also
> > > know most of users does not care. So currently I keep it as it is. But
> > > after fixing this issue on fprobe. I would like to update kretprobe so
> > > that it will use sw-breakpoint to handle it. It will increase the overhead
> > > of kretprobe, but it should be replaced by fprobe at that moment.
> >
> > Ok, given kretprobes already have this issue, can we add this support
> > for BPF multi-kprobe/kretprobe only? We can have an extra Kconfig
> > option or whatever necessary. It's sad that we don't have entire
> > feature just because a few registers can't be set (and I bet no BPF
> > users ever reads those registers from pt_regs). It's not the first,
> > nor last case where pt_regs isn't complete (e.g., tracepoints set only
> > a few fields in pt_regs, the rest are zero; and that's fine).
>
> pt_regs things are asked by PeterZ. It is not recommended to use pt_regs
> if it is not actual pt_regs because user expects it works as full pt_regs.

Yes, but it is what it is today. Tracepoints, for example, have like 4
fields set to real values and the rest are zeroes. So sure, as
complete data as possible is best, but the reality is different.

My point is, the feature being available with one or two pt_regs
fields not having a real value set is *much* better than no feature
availability. There is just no comparison here. And you said yourself,
current kretprobe implementation has similar problems, so nothing is
regressed.

> So I and Steve decided to use ftrace_regs for this faked registers.
> (I think tracepoint should also use ftrace_regs or another one.)
>
> Anyway, we're almost at a goal we can all agree on. I think we would better
> push fprobe on fgraph series instead of such ad-hoc change.

I really hope you are right, but you see yourself that there are all
the small things that pop up and need debugging, fixing,
investigation. Performance regression is pretty noticeable. So this
might take more time than all of us would like.

>
> Thank you,
>
> >
> > >
> > > Thank you,
> > >
> > > >
> > > >   [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/
> > > >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > > >
> > > > >
> > > > > Again, just letting you know.
> > > > >
> > > > > -- Steve
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Masami Hiramatsu]

On Tue, 10 Sep 2024 11:23:29 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> + arm ML and maintainers
> 
> On Wed, Sep 4, 2024 at 6:02 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > Hey,
> >
> > I just recently realized that we are still missing multi-kprobe
> > support for ARM64, which depends on CONFIG_FPROBE. And CONFIG_FPROBE
> > seems to require CONFIG_HAVE_RETHOOK, which, it turns out, is not
> > implemented for ARM64.
> >
> > It took me a while to realize what's going on, as I roughly remembered
> > (and confirmed through lore search) that Masami's original rethook
> > patches had arm64-specific bits. Long story short:
> >
> > 0f8f8030038a Revert "arm64: rethook: Add arm64 rethook implementation"
> > 83acdce68949 arm64: rethook: Add arm64 rethook implementation
> >
> > The patch was landed and then reverted. I found some discussion online
> > and it seems like the plan was to land arch-specific bits shortly
> > after bpf-next PR.
> >
> > But it seems like that never happened. Why?
> >
> > I see s390x, RISC-V, loongarch (I'm not even mentioning x86-64) all
> > have CONFIG_HAVE_RETHOOK, even powerpc is getting one (see [0]), it
> > seems. How come ARM64 is the one left out?
> >
> > Can anyone please provide some context? And if that's just an
> > oversight, can we prioritize landing this for ARM64 ASAP?
> >
> >   [0] https://lore.kernel.org/bpf/20240830113131.7597-1-adubey@linux.ibm.com/
> >
> 
> Masami, Steven,
> 
> Does Linus have to be in CC to get any reply here? Come on, it's been
> almost a full week.

Sorry about bothering you, let me check that. But I think we eventually
need my fprobe-on-fgraph patch which allows all architecture uses ftrace_regs
instead of pt_regs for ftrace/fgraph users. That allows arm64 to implement
fprobe.

> 
> Maybe ARM64 folks have some context?... And hopefully desire to see
> this through so that ARM64 doesn't stick out as a lesser-supported
> platform as far as tracing goes compared to loongarch, s390x, and
> powerpc (which just landed rethook support, see [2]).

I think lesser-supported or not is not a matter, but they need to keep 
their architecutre healthy. Mark said that the current rethook
implementation is not acceptable because arm64 can not manually generate
pt_regs. So we need to use ftrace_regs for that.
So eventually, we need my fprobe series.

https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/

Thank you,

> 
> Note that there was already an implementation (see [1]), but for some
> reason it never made it.
> 
>   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
>   [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/
> 
> >
> > -- Andrii


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Tue, Sep 10, 2024 at 5:13 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 10 Sep 2024 11:23:29 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > + arm ML and maintainers
> >
> > On Wed, Sep 4, 2024 at 6:02 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > Hey,
> > >
> > > I just recently realized that we are still missing multi-kprobe
> > > support for ARM64, which depends on CONFIG_FPROBE. And CONFIG_FPROBE
> > > seems to require CONFIG_HAVE_RETHOOK, which, it turns out, is not
> > > implemented for ARM64.
> > >
> > > It took me a while to realize what's going on, as I roughly remembered
> > > (and confirmed through lore search) that Masami's original rethook
> > > patches had arm64-specific bits. Long story short:
> > >
> > > 0f8f8030038a Revert "arm64: rethook: Add arm64 rethook implementation"
> > > 83acdce68949 arm64: rethook: Add arm64 rethook implementation
> > >
> > > The patch was landed and then reverted. I found some discussion online
> > > and it seems like the plan was to land arch-specific bits shortly
> > > after bpf-next PR.
> > >
> > > But it seems like that never happened. Why?
> > >
> > > I see s390x, RISC-V, loongarch (I'm not even mentioning x86-64) all
> > > have CONFIG_HAVE_RETHOOK, even powerpc is getting one (see [0]), it
> > > seems. How come ARM64 is the one left out?
> > >
> > > Can anyone please provide some context? And if that's just an
> > > oversight, can we prioritize landing this for ARM64 ASAP?
> > >
> > >   [0] https://lore.kernel.org/bpf/20240830113131.7597-1-adubey@linux.ibm.com/
> > >
> >
> > Masami, Steven,
> >
> > Does Linus have to be in CC to get any reply here? Come on, it's been
> > almost a full week.
>
> Sorry about bothering you, let me check that. But I think we eventually

You don't bother me, but I'd appreciate a bit more timely replies in
the future, if that's OK.

> need my fprobe-on-fgraph patch which allows all architecture uses ftrace_regs
> instead of pt_regs for ftrace/fgraph users. That allows arm64 to implement
> fprobe.

Ok, thanks for a bit more context. I understand the end goal with
fprobe-on-fgraph, but see below.

>
> >
> > Maybe ARM64 folks have some context?... And hopefully desire to see
> > this through so that ARM64 doesn't stick out as a lesser-supported
> > platform as far as tracing goes compared to loongarch, s390x, and
> > powerpc (which just landed rethook support, see [2]).
>
> I think lesser-supported or not is not a matter, but they need to keep
> their architecutre healthy. Mark said that the current rethook
> implementation is not acceptable because arm64 can not manually generate

I don't see Mark's reply in the link you sent. But did he refer to the
code in kprobes_trampoline.S or is it something different?

By lesser-supported I mean that a very important functionality (BPF
multi-kprobe, which relies on CONFIG_FPROBE and thus
{HAVE|CONFIG}_RETHOOK) is currently still missing. And whether x86-64
support landed more than 2 years ago (end of March 2022), the second
practically most popular (and thus important for tools and such) ARM64
platform still doesn't have this functionality.

And that's limiting, BPF multi-kprobes are a huge improvement in
tooling usability. So while I get the desire to have a clean and nice
end goal, and that it might take a bit longer to get everything right.
But, maybe, landing a stop-gap solution meanwhile (especially as
isolated and thus easily backportable as the patch [0] you referenced)
is an OK path forward?

I'm just lacking full understanding on what exactly the issue is/was,
and that's why I'm asking all these questions. I'm not sure if [0] is
just broken for some subtle reason, or it is just suboptimal in some
sense (performance, code duplication, whatnot)?


  [0] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/

> pt_regs. So we need to use ftrace_regs for that.
> So eventually, we need my fprobe series.
>
> https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
>
> Thank you,
>
> >
> > Note that there was already an implementation (see [1]), but for some
> > reason it never made it.
> >
> >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> >   [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/
> >
> > >
> > > -- Andrii
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Masami Hiramatsu]

On Tue, 10 Sep 2024 17:37:48 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Tue, Sep 10, 2024 at 5:13 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 10 Sep 2024 11:23:29 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > + arm ML and maintainers
> > >
> > > On Wed, Sep 4, 2024 at 6:02 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > Hey,
> > > >
> > > > I just recently realized that we are still missing multi-kprobe
> > > > support for ARM64, which depends on CONFIG_FPROBE. And CONFIG_FPROBE
> > > > seems to require CONFIG_HAVE_RETHOOK, which, it turns out, is not
> > > > implemented for ARM64.
> > > >
> > > > It took me a while to realize what's going on, as I roughly remembered
> > > > (and confirmed through lore search) that Masami's original rethook
> > > > patches had arm64-specific bits. Long story short:
> > > >
> > > > 0f8f8030038a Revert "arm64: rethook: Add arm64 rethook implementation"
> > > > 83acdce68949 arm64: rethook: Add arm64 rethook implementation
> > > >
> > > > The patch was landed and then reverted. I found some discussion online
> > > > and it seems like the plan was to land arch-specific bits shortly
> > > > after bpf-next PR.
> > > >
> > > > But it seems like that never happened. Why?
> > > >
> > > > I see s390x, RISC-V, loongarch (I'm not even mentioning x86-64) all
> > > > have CONFIG_HAVE_RETHOOK, even powerpc is getting one (see [0]), it
> > > > seems. How come ARM64 is the one left out?
> > > >
> > > > Can anyone please provide some context? And if that's just an
> > > > oversight, can we prioritize landing this for ARM64 ASAP?
> > > >
> > > >   [0] https://lore.kernel.org/bpf/20240830113131.7597-1-adubey@linux.ibm.com/
> > > >
> > >
> > > Masami, Steven,
> > >
> > > Does Linus have to be in CC to get any reply here? Come on, it's been
> > > almost a full week.
> >
> > Sorry about bothering you, let me check that. But I think we eventually
> 
> You don't bother me, but I'd appreciate a bit more timely replies in
> the future, if that's OK.
> 
> > need my fprobe-on-fgraph patch which allows all architecture uses ftrace_regs
> > instead of pt_regs for ftrace/fgraph users. That allows arm64 to implement
> > fprobe.
> 
> Ok, thanks for a bit more context. I understand the end goal with
> fprobe-on-fgraph, but see below.
> 
> >
> > >
> > > Maybe ARM64 folks have some context?... And hopefully desire to see
> > > this through so that ARM64 doesn't stick out as a lesser-supported
> > > platform as far as tracing goes compared to loongarch, s390x, and
> > > powerpc (which just landed rethook support, see [2]).
> >
> > I think lesser-supported or not is not a matter, but they need to keep
> > their architecutre healthy. Mark said that the current rethook
> > implementation is not acceptable because arm64 can not manually generate
> 
> I don't see Mark's reply in the link you sent. But did he refer to the
> code in kprobes_trampoline.S or is it something different?

Sorry, here it is: https://lkml.org/lkml/2022/4/12/2233

> 
> By lesser-supported I mean that a very important functionality (BPF
> multi-kprobe, which relies on CONFIG_FPROBE and thus
> {HAVE|CONFIG}_RETHOOK) is currently still missing. And whether x86-64
> support landed more than 2 years ago (end of March 2022), the second
> practically most popular (and thus important for tools and such) ARM64
> platform still doesn't have this functionality.
> 
> And that's limiting, BPF multi-kprobes are a huge improvement in
> tooling usability.

Sorry for inconvenient. But I think this transformation is really
important.

> So while I get the desire to have a clean and nice
> end goal, and that it might take a bit longer to get everything right.
> But, maybe, landing a stop-gap solution meanwhile (especially as
> isolated and thus easily backportable as the patch [0] you referenced)
> is an OK path forward?

I had not realized that the PSTATE register was not saved correctly
at that point. This is one reason why I decided to move in the
current fprobe-on-fgraph direction.

> 
> I'm just lacking full understanding on what exactly the issue is/was,
> and that's why I'm asking all these questions. I'm not sure if [0] is
> just broken for some subtle reason, or it is just suboptimal in some
> sense (performance, code duplication, whatnot)?

If [0] was not broken, I pushed it and the current pt_regs to ftrace_regs
series is separated series. But it was broken. So I tried to find the
correct way to fix it, and finally introduced the current fprobe on 
fgraph series. performance improvement is just a side effect.

Thank you,

> 
> 
>   [0] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> 
> > pt_regs. So we need to use ftrace_regs for that.
> > So eventually, we need my fprobe series.
> >
> > https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> >
> > Thank you,
> >
> > >
> > > Note that there was already an implementation (see [1]), but for some
> > > reason it never made it.
> > >
> > >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > >   [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/
> > >
> > > >
> > > > -- Andrii
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Wed, Sep 11, 2024 at 8:18 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 10 Sep 2024 17:37:48 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Tue, Sep 10, 2024 at 5:13 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > On Tue, 10 Sep 2024 11:23:29 -0700
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >
> > > > + arm ML and maintainers
> > > >
> > > > On Wed, Sep 4, 2024 at 6:02 PM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > > >
> > > > > Hey,
> > > > >
> > > > > I just recently realized that we are still missing multi-kprobe
> > > > > support for ARM64, which depends on CONFIG_FPROBE. And CONFIG_FPROBE
> > > > > seems to require CONFIG_HAVE_RETHOOK, which, it turns out, is not
> > > > > implemented for ARM64.
> > > > >
> > > > > It took me a while to realize what's going on, as I roughly remembered
> > > > > (and confirmed through lore search) that Masami's original rethook
> > > > > patches had arm64-specific bits. Long story short:
> > > > >
> > > > > 0f8f8030038a Revert "arm64: rethook: Add arm64 rethook implementation"
> > > > > 83acdce68949 arm64: rethook: Add arm64 rethook implementation
> > > > >
> > > > > The patch was landed and then reverted. I found some discussion online
> > > > > and it seems like the plan was to land arch-specific bits shortly
> > > > > after bpf-next PR.
> > > > >
> > > > > But it seems like that never happened. Why?
> > > > >
> > > > > I see s390x, RISC-V, loongarch (I'm not even mentioning x86-64) all
> > > > > have CONFIG_HAVE_RETHOOK, even powerpc is getting one (see [0]), it
> > > > > seems. How come ARM64 is the one left out?
> > > > >
> > > > > Can anyone please provide some context? And if that's just an
> > > > > oversight, can we prioritize landing this for ARM64 ASAP?
> > > > >
> > > > >   [0] https://lore.kernel.org/bpf/20240830113131.7597-1-adubey@linux.ibm.com/
> > > > >
> > > >
> > > > Masami, Steven,
> > > >
> > > > Does Linus have to be in CC to get any reply here? Come on, it's been
> > > > almost a full week.
> > >
> > > Sorry about bothering you, let me check that. But I think we eventually
> >
> > You don't bother me, but I'd appreciate a bit more timely replies in
> > the future, if that's OK.
> >
> > > need my fprobe-on-fgraph patch which allows all architecture uses ftrace_regs
> > > instead of pt_regs for ftrace/fgraph users. That allows arm64 to implement
> > > fprobe.
> >
> > Ok, thanks for a bit more context. I understand the end goal with
> > fprobe-on-fgraph, but see below.
> >
> > >
> > > >
> > > > Maybe ARM64 folks have some context?... And hopefully desire to see
> > > > this through so that ARM64 doesn't stick out as a lesser-supported
> > > > platform as far as tracing goes compared to loongarch, s390x, and
> > > > powerpc (which just landed rethook support, see [2]).
> > >
> > > I think lesser-supported or not is not a matter, but they need to keep
> > > their architecutre healthy. Mark said that the current rethook
> > > implementation is not acceptable because arm64 can not manually generate
> >
> > I don't see Mark's reply in the link you sent. But did he refer to the
> > code in kprobes_trampoline.S or is it something different?
>
> Sorry, here it is: https://lkml.org/lkml/2022/4/12/2233
>

Thanks for the link!

> >
> > By lesser-supported I mean that a very important functionality (BPF
> > multi-kprobe, which relies on CONFIG_FPROBE and thus
> > {HAVE|CONFIG}_RETHOOK) is currently still missing. And whether x86-64
> > support landed more than 2 years ago (end of March 2022), the second
> > practically most popular (and thus important for tools and such) ARM64
> > platform still doesn't have this functionality.
> >
> > And that's limiting, BPF multi-kprobes are a huge improvement in
> > tooling usability.
>
> Sorry for inconvenient. But I think this transformation is really
> important.
>
> > So while I get the desire to have a clean and nice
> > end goal, and that it might take a bit longer to get everything right.
> > But, maybe, landing a stop-gap solution meanwhile (especially as
> > isolated and thus easily backportable as the patch [0] you referenced)
> > is an OK path forward?
>
> I had not realized that the PSTATE register was not saved correctly
> at that point. This is one reason why I decided to move in the
> current fprobe-on-fgraph direction.

Sure, but you said yourself, the same problem exists with current
kretprobe implementation, so this won't regress anything. And yes,
your fprobe-on-fgraph series is supposed to fix this for good, which
is great, but that's a separate topic.

>
> >
> > I'm just lacking full understanding on what exactly the issue is/was,
> > and that's why I'm asking all these questions. I'm not sure if [0] is
> > just broken for some subtle reason, or it is just suboptimal in some
> > sense (performance, code duplication, whatnot)?
>
> If [0] was not broken, I pushed it and the current pt_regs to ftrace_regs
> series is separated series. But it was broken. So I tried to find the
> correct way to fix it, and finally introduced the current fprobe on
> fgraph series. performance improvement is just a side effect.

So, looking at Mark's replies, I didn't get the impression that
something was badly broken (but what do I know about arm64 bits). The
reason I'm asking to implement rethook on arm64 is because the code is
almost there, and so it seems like it would be pretty easy to get this
in. The code doesn't regress anything compared to existing kretprobe
implementation. And there isn't that much code and it seems to be
pretty well contained. This makes it easier to backport to older
production kernels. Which is great thing all in itself.

Mark, what are your thoughts on this? Would you be ok with landing
rethook-for-arm64 as an interim solution while Masami is ironing out
the kinds with his fprobe-on-fgraph solution (which seems to have a
pretty noticeable regression for kprobes, and I'm not sure that's
acceptable; this is being investigated at the moment, cc @Jiri).

>
> Thank you,
>
> >
> >
> >   [0] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> >
> > > pt_regs. So we need to use ftrace_regs for that.
> > > So eventually, we need my fprobe series.
> > >
> > > https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > >
> > > Thank you,
> > >
> > > >
> > > > Note that there was already an implementation (see [1]), but for some
> > > > reason it never made it.
> > > >
> > > >   [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/
> > > >   [2] https://lore.kernel.org/bpf/172562357215.467568.2172858907419105155.b4-ty@ellerman.id.au/
> > > >
> > > > >
> > > > > -- Andrii
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Masami Hiramatsu]

On Wed, 11 Sep 2024 13:18:12 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> > > So while I get the desire to have a clean and nice
> > > end goal, and that it might take a bit longer to get everything right.
> > > But, maybe, landing a stop-gap solution meanwhile (especially as
> > > isolated and thus easily backportable as the patch [0] you referenced)
> > > is an OK path forward?
> >
> > I had not realized that the PSTATE register was not saved correctly
> > at that point. This is one reason why I decided to move in the
> > current fprobe-on-fgraph direction.
> 
> Sure, but you said yourself, the same problem exists with current
> kretprobe implementation, so this won't regress anything. And yes,
> your fprobe-on-fgraph series is supposed to fix this for good, which
> is great, but that's a separate topic.

It does not regress kretprobe, but introduces the same problem to
fprobe. And since fprobe-on-fgraph was boosted by this problem,
I think that is not a separate topic.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: Unsupported CONFIG_FPROBE and CONFIG_RETHOOK on ARM64

[Andrii Nakryiko]

On Wed, Sep 11, 2024 at 4:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed, 11 Sep 2024 13:18:12 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > > > So while I get the desire to have a clean and nice
> > > > end goal, and that it might take a bit longer to get everything right.
> > > > But, maybe, landing a stop-gap solution meanwhile (especially as
> > > > isolated and thus easily backportable as the patch [0] you referenced)
> > > > is an OK path forward?
> > >
> > > I had not realized that the PSTATE register was not saved correctly
> > > at that point. This is one reason why I decided to move in the
> > > current fprobe-on-fgraph direction.
> >
> > Sure, but you said yourself, the same problem exists with current
> > kretprobe implementation, so this won't regress anything. And yes,
> > your fprobe-on-fgraph series is supposed to fix this for good, which
> > is great, but that's a separate topic.
>
> It does not regress kretprobe, but introduces the same problem to
> fprobe.

I'm not sure if we are on the same page. There is no FPROBE on arm64
right now due to lack of HAVE_RETHOOK. Implementing rethook on ARM64
quickly will enable FPROBE on ARM64 and make it so that this can be
pretty easily backported to old kernels. Yes, PSTATE register won't be
complete (we can set it to zero or whatever to make this more
obvious), but then an entire BPF multi-kprobe functionality will start
working on ARM64.

In my mind it's undoubtedly **much better** to not have PSTATE value
and have FPROBE and thus BPF multi-kprobes than not having anything at
all.

> And since fprobe-on-fgraph was boosted by this problem,
> I think that is not a separate topic.

I see fprobe-on-fgraph as an improvement on top of implementing
RETHOOK for ARM64, it doesn't have to block and preclude it. I'm quite
puzzled that PSTATE register value is a big deal for you, but 15%
performance drop (for anyone following, see [1]) due to
fprobe-on-fgraph is somehow not. Quoting you from [0]:

  > Anyway, performance optimization can be done
  > afterwards, so I'm not so worried it :)

If "afterwards" means before we turn on fprobe-on-fgraph for x86-64,
then sure. But otherwise I'm not sure it's acceptable to just have a
15% regression just like that.

  [0] https://lore.kernel.org/bpf/20240912100928.a7322dc9161a90aa723662c4@kernel.org/
  [1] https://lore.kernel.org/bpf/ZuHhD35xHpw2kCC-@krava/

>
> Thank you,
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>