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

[Rémi Denis-Courmont]

Le maanantaina 17. heinäkuuta 2023, 20.48.40 EEST Lynne a écrit :
> >> > But I still argue that that is, either way, completely negligible
> >> > compared
> >> > to the *existing* overhead. Each loop is making 4 system calls, and
> >> > each
> >> > of those system call requires a direct call (to PLT) and an indirect
> >> > branch (from GOT). If you have a problem with the two additional
> >> > function
> >> > calls, then you can't be using Linux perf in the first place.
> >> 
> >> You don't want to ever use linux perf in the first place, it's second
> >> class.> 
> > No it isn't. The interface is more involved than just reading a CSR; and
> > sure I'd prefer the simple interface that RDCYCLE is all other things
> > being equal. But other things are not equal. Linux perf is in fact *more*
> > accurate by virtue of not *wrongly* counting other things. And it does
> > not threaten the security of the entire system, so it will work inside a
> > rented VM or an unprivileged process.
> 
> Threaten?

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.

> 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.

> If anyone doesn't want to use rdcycle, they can use linux perf, it still
> works, with or without the patch.

It does not.

> >> I don't think it's worth changing the direct inlining we had before.
> >> You're
> >> not interested in whether or not the same exact code is ran between
> >> platforms,
> > 
> > Err, I am definitely interested in doing exactly that. I don't want to
> > have to reconfigure and recompile the entire FFmpeg just to switch
> > between Linux perf and raw cycle counter. A contrario, I *do* want to
> > compare performance between vendors once the hardware is available.
> 
> That's a weak reason to compromise the accuracy of a development tool.

This is not compromising any accuracy of any development tool. Again, in a 
scenario where both RDCYCLE and Linux perf work, Linux perf is more accurate 
for reasons that I already outlined in previous messages. And on systems with 
newer OpenSBI and kernel, RDCYCLE does not work at all.

> >> just that the code that's measuring timing is as efficient and
> >> low overhead as possible.
> > 
> > Of course not. Low overhead is irrelevant here. The measurement overhead
> > is know and is subtracted. What we need is stable/reproducible overhead,
> > and accurate measurements.
> 
> Which is what TSC or the equivalent gets you. It's noisy, but that's because
> it's better and higher accuracy than having to roundtrip through the
> kernel.

We _could_ use RDTIME. That's not blocked.

And while that should be "low overhead", but measuring time is also obviously 
way _less_ accurate than measuring cycles (RDCYCLE), which is in turn _less_ 
accurate than measuring cycles only in user mode and only of the current 
process (Linux perf).

> 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.

All you've done is make it abundantly clear that you don't like Linux perf and 
don't care about the rationale for this patch because it doesn't suit your 
personal preferences, especially on IRC:

20:49 <@Lynne> "security"
20:49 <@Lynne> it's a fucking timer
20:49 <@Lynne> "insecure"
20:51 <@Lynne> computers are very good at knowing how much time has passed, to 
               the point of the speed of light being an issue
20:52 <@Lynne> getting rid of this because some script kiddie could 
potentially 
               figure out a bit by calling this a million times while the CPU 
               is busy is paranoid
20:58 <@Lynne> it's a cache issue, so you fix the cache lifetime, not nerf a 
               timer

And it's not a cache issue. Cycle Drift is not a cache issue, it's an 
instruction timing issue.

Since you're giving zero valid reasons, I'm invoking the TC.

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