diff mbox series

[FFmpeg-devel,2/2] avutil/macos_kperf: Fix assert which makes kperf failed to run

Message ID tencent_8C54698E039569F27C0AAD9A3979C1E1B50A@qq.com
State New
Headers show
Series [FFmpeg-devel,1/2] avutil/timer: define macos kperf as AV_READ_TIME | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Zhao Zhili June 12, 2024, 3:22 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
doesn't matter in our case.
---
 libavutil/macos_kperf.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Martin Storsjö June 17, 2024, 11:10 a.m. UTC | #1
On Wed, 12 Jun 2024, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
> doesn't matter in our case.

This is somewhat unexpected, I had expected that this API was originally 
tested on an m1. I guess it might depend on what OS version you're using 
as well?

> ---
> libavutil/macos_kperf.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
> index a0bc845fd3..906b276a34 100644
> --- a/libavutil/macos_kperf.c
> +++ b/libavutil/macos_kperf.c
> @@ -67,14 +67,15 @@ KPERF_LIST
> #define KPC_CLASS_POWER_MASK        (1 << 2)
> #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
>
> -#define COUNTERS_COUNT 10
> +#define KPC_MAX_COUNTERS 32
> #define CONFIG_COUNT 8
> #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
>
> static void kperf_init(void)
> {
> -    uint64_t config[COUNTERS_COUNT] = {0};
> +    uint64_t config[CONFIG_COUNT] = {0};

Hmm, this changes the array from 10 to 8 elements. While the change looks 
reasonable based on the variable names, I just wanted to doublecheck that 
we have some clues that this is right?

>     void *kperf = NULL;
> +    uint32_t n;
>
>     av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
>
> @@ -82,8 +83,10 @@ static void kperf_init(void)
>     KPERF_LIST
> #undef F
>
> -    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
> -    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
> +    n = kpc_get_counter_count(KPC_MASK);
> +    av_assert0(n <= KPC_MAX_COUNTERS);
> +    n = kpc_get_config_count(KPC_MASK);
> +    av_assert0(n <= CONFIG_COUNT);

I guess this is the actual functional change here, I think this seems 
right.

I CC's Josh on this change too, in case he has something to add here, but 
it looks mostly reasonable to me.

// Martin
J. Dekker June 17, 2024, 11:45 a.m. UTC | #2
On Mon, Jun 17, 2024, at 13:10, Martin Storsjö wrote:
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
>
>> From: Zhao Zhili <zhilizhao@tencent.com>
>>
>> On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
>> doesn't matter in our case.
>
> This is somewhat unexpected, I had expected that this API was originally 
> tested on an m1. I guess it might depend on what OS version you're using 
> as well?
>
>> ---
>> libavutil/macos_kperf.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
>> index a0bc845fd3..906b276a34 100644
>> --- a/libavutil/macos_kperf.c
>> +++ b/libavutil/macos_kperf.c
>> @@ -67,14 +67,15 @@ KPERF_LIST
>> #define KPC_CLASS_POWER_MASK        (1 << 2)
>> #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
>>
>> -#define COUNTERS_COUNT 10
>> +#define KPC_MAX_COUNTERS 32
>> #define CONFIG_COUNT 8
>> #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
>>
>> static void kperf_init(void)
>> {
>> -    uint64_t config[COUNTERS_COUNT] = {0};
>> +    uint64_t config[CONFIG_COUNT] = {0};
>
> Hmm, this changes the array from 10 to 8 elements. While the change looks 
> reasonable based on the variable names, I just wanted to doublecheck that 
> we have some clues that this is right?
>
>>     void *kperf = NULL;
>> +    uint32_t n;
>>
>>     av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
>>
>> @@ -82,8 +83,10 @@ static void kperf_init(void)
>>     KPERF_LIST
>> #undef F
>>
>> -    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
>> -    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
>> +    n = kpc_get_counter_count(KPC_MASK);
>> +    av_assert0(n <= KPC_MAX_COUNTERS);
>> +    n = kpc_get_config_count(KPC_MASK);
>> +    av_assert0(n <= CONFIG_COUNT);
>
> I guess this is the actual functional change here, I think this seems 
> right.
>
> I CC's Josh on this change too, in case he has something to add here, but 
> it looks mostly reasonable to me.

It’s a private kernel API there are no guarantees about it working at any point. I just know that it used to work. If it needs changes to work currently and this fixes it for you then that’s fine, you should probably also port this change to dav1d. I’m personally unable to test this as I am just running Linux on my M1 machine now.
Zhao Zhili June 17, 2024, 12:20 p.m. UTC | #3
> On Jun 17, 2024, at 19:10, Martin Storsjö <martin@martin.st> wrote:
> 
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
>> doesn't matter in our case.
> 
> This is somewhat unexpected, I had expected that this API was originally tested on an m1. I guess it might depend on what OS version you're using as well?

On arm64 [1]

#define KPC_ARM64_FIXED_COUNT        (2)
#define KPC_ARM64_CONFIGURABLE_COUNT (CORE_NCTRS - KPC_ARM64_FIXED_COUNT)

#define KPC_MAX_COUNTERS (KPC_ARM64_FIXED_COUNT + KPC_ARM64_CONFIGURABLE_COUNT + 1)

On x86_64:
#define KPC_MAX_COUNTERS 32

[1] https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/osfmk/arm64/machine_kpc.h#L36

> 
>> ---
>> libavutil/macos_kperf.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
>> index a0bc845fd3..906b276a34 100644
>> --- a/libavutil/macos_kperf.c
>> +++ b/libavutil/macos_kperf.c
>> @@ -67,14 +67,15 @@ KPERF_LIST
>> #define KPC_CLASS_POWER_MASK        (1 << 2)
>> #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
>> 
>> -#define COUNTERS_COUNT 10
>> +#define KPC_MAX_COUNTERS 32
>> #define CONFIG_COUNT 8
>> #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
>> 
>> static void kperf_init(void)
>> {
>> -    uint64_t config[COUNTERS_COUNT] = {0};
>> +    uint64_t config[CONFIG_COUNT] = {0};
> 
> Hmm, this changes the array from 10 to 8 elements. While the change looks reasonable based on the variable names, I just wanted to doublecheck that we have some clues that this is right?

The change is base on the check

av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT

> 
>>    void *kperf = NULL;
>> +    uint32_t n;
>> 
>>    av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
>> 
>> @@ -82,8 +83,10 @@ static void kperf_init(void)
>>    KPERF_LIST
>> #undef F
>> 
>> -    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
>> -    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
>> +    n = kpc_get_counter_count(KPC_MASK);
>> +    av_assert0(n <= KPC_MAX_COUNTERS);
>> +    n = kpc_get_config_count(KPC_MASK);
>> +    av_assert0(n <= CONFIG_COUNT);
> 
> I guess this is the actual functional change here, I think this seems right.
> 
> I CC's Josh on this change too, in case he has something to add here, but it looks mostly reasonable to me.

> 
> // Martin
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
index a0bc845fd3..906b276a34 100644
--- a/libavutil/macos_kperf.c
+++ b/libavutil/macos_kperf.c
@@ -67,14 +67,15 @@  KPERF_LIST
 #define KPC_CLASS_POWER_MASK        (1 << 2)
 #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
 
-#define COUNTERS_COUNT 10
+#define KPC_MAX_COUNTERS 32
 #define CONFIG_COUNT 8
 #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
 
 static void kperf_init(void)
 {
-    uint64_t config[COUNTERS_COUNT] = {0};
+    uint64_t config[CONFIG_COUNT] = {0};
     void *kperf = NULL;
+    uint32_t n;
 
     av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
 
@@ -82,8 +83,10 @@  static void kperf_init(void)
     KPERF_LIST
 #undef F
 
-    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
-    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
+    n = kpc_get_counter_count(KPC_MASK);
+    av_assert0(n <= KPC_MAX_COUNTERS);
+    n = kpc_get_config_count(KPC_MASK);
+    av_assert0(n <= CONFIG_COUNT);
 
     config[0] = CPMU_CORE_CYCLE | CFGWORD_EL0A64EN_MASK;
     // config[3] = CPMU_INST_BRANCH | CFGWORD_EL0A64EN_MASK;
@@ -99,11 +102,11 @@  static void kperf_init(void)
 uint64_t ff_kperf_cycles(void)
 {
     static AVOnce init_static_once = AV_ONCE_INIT;
-    uint64_t counters[COUNTERS_COUNT];
+    uint64_t counters[KPC_MAX_COUNTERS];
 
     ff_thread_once(&init_static_once, kperf_init);
 
-    if (kpc_get_thread_counters(0, COUNTERS_COUNT, counters)) {
+    if (kpc_get_thread_counters(0, KPC_MAX_COUNTERS, counters)) {
         return -1;
     }