[FFmpeg-devel] [PATCH 4/7] checkasm: use pointers for start/stop functions

[Rémi Denis-Courmont]

Le keskiviikkona 19. heinäkuuta 2023, 0.32.26 EEST Lynne a écrit :
> > User-space access to the cycle counter has been deemed a security threat
> > due to the Cycle Drift attack, and is disabled as of OpenSBI 1.3.
> > 
> > If FFmpeg does not support Linux perf, FFmpeg will get _no_ performance
> > benchmarks on Linux.
> 
> It does support linux perf. I've used it many times.

Yes of course. This MR modifies the Linux perf code, so I can see that it's 
there. My point is that *if* Linux perf is *not* enabled (in ./configure), 
there will be no benchmarks anymore on newer Linux RISC-V systems. But if it 
is enabled it, there will be no benchmarks on existing systems. Hence the need 
for the run-time indirection, or some very major PITA for development and 
supporting new developers (and we've seen already 3 different people sending 
RISC-V patches besides me in the past few months).

> >> This is a development tool first and foremost.
> > 
> > A development tool is not a justification for leaving a security whole in
> > the system. I don't make the rules, and you don't either. OpenSBI and
> > Linux make them.
> 
> You're not required to leave them on always. It's a development tool.
> You don't disable frequency scaling on all the time on other platforms,
> just when doing benchmarks, which is what the tool is meant for.

There is no need to disable frequency scaling if you use the most _suited_ 
counter for microbencmarks, that is to say the cycle counter. And then, 
disabling frequency scaling is not always feasible.

> For ARM, we routinely use an external kernel module to enable
> the performance timers just so we avoid linux perf.

I don't think an external ARM-specific kernel module is an excuse for breaking 
checkasm for _normal_ out-of-the-box kernel setups. Just because checkasm is a 
development tool doesn't mean that it only runs on custom development systems.

> Specifically on arm, the linux perf noise is very high, which is where
> my concerns are.

I fail to see how this MR makes this any worse. You can still disable Linux 
perf at build time as you could before. Quite the contrary in fact: if the 
noise is so high then there are no ways that a single indirect function call 
would make any meaningful difference for the worse.

> >> If anyone doesn't want to use rdcycle, they can use linux perf, it still
> >> works, with or without the patch.
> > 
> > It does not.
> 
> Why not? It works on arm. The same exact code is used on risc-v
> and on any other platform too.

Currently it's disabled by default on RISC-V (really anything but Arm) in 
FFmpeg's configure, and hardly anybody has the necessary OpenSBI and Linux 
updates for Linux perf to work on RISC-V.

We can't simply assume that Linux perf is supported. We're a few years too 
early for that. And by then, Android will have blocked Linux perf, so we will 
have the same problem anyway, only for different reasons.

> >> Either way, I don't agree with this patch, not accepting it.
> > 
> > The only vaguely valid reason you've given is that this should cache the
> > function pointers locally, which version 2 does.
> 
> I disagree with it doing an indirection at all.

I can see that, and you're entitled to your opinion.
But that's neither technical reasoning, nor actionable review feedback.

> It's overhead and a potential source of inaccuracy.

If Linux perf is enabled, it's blatantly obvious that whatever additional 
overhead this adds is negligible. Since you insist, on top of my head, Linux 
perf already now will do quite a few indirect calls:
- from checkasm's PLT to libc's ioctl(),
- from libc's syscall to kernel exception vector,
- from exception vector to ioctl syscall handler,
- from ioctl handler to PMU framework,
- from PMU framework to PMU backend driver.
And that's grossly over-simplified and glossing over all the other calls and 
stuff that happen in-between.

As for the case that Linux perf is disabled at build time, then I can make a 
different version that calls AV_READ_TIME directly without indirection if 
that's what bothers Gramner and/or you. Though you've noted yourself that that 
path is also very noisy as it is so not like adding one indirect call will 
matter here :shrug:

> It's up to you to prove it wouldn't affect major platforms.

I think that I have supplied plenty of technical arguments that there 
shouldn't be a problem with this MR. And in the unlikely event that there 
would be an unexpected problem, I'm sure somebody would revert it; it's not an 
irreversible situation. Not to mention that, in the mean time, Martin did 
indeed verify that the overhead is small and stable (on Arm).

> > Since you're giving zero valid reasons, I'm invoking the TC.
> 
> What, you expect them to materialize an opinion out of thin air?

I think that they already have plenty to materialise an opinion from, between 
the MR, this thread, Martin's benchmarks, and their experience. Otherwise, 
they always have the possibility to ask for clarifications (it's even in the 
community rulebook).

> You could've asked the actual maintainer, and one who knows the
> most about the code, Gramner.

Henrik was and still is welcome to review and comment. I am not in the habit 
of asking permission to send code for review.

> But you're focusing too hard on arguing about irrelevant issues, even
> bringing up security, rather than using reasoning or clearly describing the
> need for the patch.

Security is the reason why RDCYCLE will stop working, and that in turn is the 
motivation for this set. I believe it's well described in the commit message 
(notably patch 6/7), and this is the first time in this thread that you state 
otherwise. You don't like the security restriction and you don't have to. But 
that does not make it "irrelevant".

And if you want to argue against the change and/or its security motivation, 
you need to make your case on Linux RISC-V, Linux pref and Linux kernel 
mailing lists, not here.

> By the way, if you're going to start doing personal attacks,

Quoting somebody is not attacking them. I didn't even slice or edit the quote 
or otherwise attempt to distort it.

> right down to bringing up random quotes from the IRC,
> unrelated to you,

It looked very much like it was related to the *MR*. Maybe it was just a 
"random" coincidence, and I am sorry that you apparently took so badly to 
being quoted.

> I would ask the community committee to step in.
> Those, on the other hand, are well-known figures.

-- 
レミ・デニ-クールモン
http://www.remlab.net/