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

[Lynne]

Jul 17, 2023, 20:09 by remi at remlab.net:

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

It does support linux perf. I've used it many times.


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

For ARM, we routinely use an external kernel module to enable
the performance timers just so we avoid linux perf.
Specifically on arm, the linux perf noise is very high, which is where
my concerns are.


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


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

I disagree with it doing an indirection at all.
It's overhead and a potential source of inaccuracy.
It's up to you to prove it wouldn't affect major platforms.


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

What, you expect them to materialize an opinion out of thin air?
You have to call for a vote. And bring up why it hasn't been changed
in 3 years, even though in the duties listed, it's only supposed
to last for a year before elections are called again. Which possibly
involves doing elections again.

You could've asked the actual maintainer, and one who knows the
most about the code, Gramner. 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.

> 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:
By the way, if you're going to start doing personal attacks, right down
to bringing up random quotes from the IRC, unrelated to you, I would
ask the community committee to step in. Those, on the other hand,
are well-known figures.