Re: [RFC] drm/amdkfd: Use logical cpu id for building vcrat

Re: [RFC] drm/amdkfd: Use logical cpu id for building vcrat

[Kuehling, Felix]

[-- Attachment #1.1: Type: text/plain, Size: 2670 bytes --]

On x86 we use the apicid to associate caches with CPU cores. See the Thunk code in libhsakmt/src/topology.c (static void find_cpu_cache_siblings()). If we used a different way to identify CPU cores, I think that would break. This code in the Thunk is x86-specific as it uses the CPUID instruction. We don't have equivalent code for ARM. So for ARM it doesn't really matter much, how you count your CPU cores in the CRAT table.

I think eventually we want to get rid of that fragile CPUID code in the Thunk and get the cache information in kernel mode and report it to user mode through the KFD topology sysfs filesystem. Then we could also move away from using apicids as CPU IDs on x86.

It's not a high priority as I'm not aware of any applications that actually make use of the cache information.

Regards,
  Felix

On 2019-04-15 22:39, Hillf Danton wrote:
Hi folks

In commit d1c234e2cd, arm64 is granted to build kfd. Currently, it is physical
cpu id that is used for building the x86_64 vcrat, but logical cpu id is used
instead for arm64, though the function name requires apicid. Can we use the
physical id for both arches if it really has an up-hand over the logical one,
as the following tiny diff represents?

--- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c    2019-04-16 07:55:56.611685400 +0800
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 09:16:50.506126600 +0800
@@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
       first_cpu_of_numa_node = cpumask_first(cpumask);
       if (first_cpu_of_numa_node >= nr_cpu_ids)
             return -1;
-#ifdef CONFIG_X86_64
-     return cpu_data(first_cpu_of_numa_node).apicid;
-#else
-     return first_cpu_of_numa_node;
-#endif
+    return cpu_physical_id(first_cpu_of_numa_node);
}
/* kfd_numa_node_to_apic_id - Returns the APIC ID of the first logical processor
--


Or is logical cpu id enough to do the work, with some cosmetic applied to the
function names(not included in the following simple diff yet)?

thanks
Hillf


--- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c    2019-04-16 07:55:56.611685400 +0800
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 09:18:24.546578400 +0800
@@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
       first_cpu_of_numa_node = cpumask_first(cpumask);
       if (first_cpu_of_numa_node >= nr_cpu_ids)
             return -1;
-#ifdef CONFIG_X86_64
-     return cpu_data(first_cpu_of_numa_node).apicid;
-#else
       return first_cpu_of_numa_node;
-#endif
}
/* kfd_numa_node_to_apic_id - Returns the APIC ID of the first logical processor
--


[-- Attachment #1.2: Type: text/html, Size: 6865 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] drm/amdkfd: Use logical cpu id for building vcrat

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 3480 bytes --]

> It's not a high priority as I'm not aware of any applications that 
> actually make use of the cache information.
>
Which raises the question why we have done this in the first place? When 
nobody is using it could we just remove the interface?

Regards,
Christian.

Am 16.04.19 um 05:24 schrieb Kuehling, Felix:
>
> On x86 we use the apicid to associate caches with CPU cores. See the 
> Thunk code in libhsakmt/src/topology.c (static void 
> find_cpu_cache_siblings()). If we used a different way to identify CPU 
> cores, I think that would break. This code in the Thunk is 
> x86-specific as it uses the CPUID instruction. We don't have 
> equivalent code for ARM. So for ARM it doesn't really matter much, how 
> you count your CPU cores in the CRAT table.
>
> I think eventually we want to get rid of that fragile CPUID code in 
> the Thunk and get the cache information in kernel mode and report it 
> to user mode through the KFD topology sysfs filesystem. Then we could 
> also move away from using apicids as CPU IDs on x86.
>
> It's not a high priority as I'm not aware of any applications that 
> actually make use of the cache information.
>
> Regards,
>   Felix
>
> On 2019-04-15 22:39, Hillf Danton wrote:
>>
>> Hi folks
>>
>> In commit d1c234e2cd, arm64 is granted to build kfd. Currently, it is 
>> physical
>>
>> cpu id that is used for building the x86_64 vcrat, but logical cpu id 
>> is used
>>
>> instead for arm64, though the function name requires apicid. Can we 
>> use the
>>
>> physical id for both arches if it really has an up-hand over the 
>> logical one,
>>
>> as the following tiny diff represents?
>>
>> --- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
>> 2019-04-16 07:55:56.611685400 +0800
>>
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 
>> 09:16:50.506126600 +0800
>>
>> @@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
>>
>> first_cpu_of_numa_node = cpumask_first(cpumask);
>>
>>        if (first_cpu_of_numa_node >= nr_cpu_ids)
>>
>>              return -1;
>>
>> -#ifdef CONFIG_X86_64
>>
>> -     return cpu_data(first_cpu_of_numa_node).apicid;
>>
>> -#else
>>
>> -     return first_cpu_of_numa_node;
>>
>> -#endif
>>
>> +    return cpu_physical_id(first_cpu_of_numa_node);
>>
>> }
>>
>> /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first 
>> logical processor
>>
>> --
>>
>> Or is logical cpu id enough to do the work, with some cosmetic 
>> applied to the
>>
>> function names(not included in the following simple diff yet)?
>>
>> thanks
>>
>> Hillf
>>
>> --- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
>> 2019-04-16 07:55:56.611685400 +0800
>>
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 
>> 09:18:24.546578400 +0800
>>
>> @@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
>>
>> first_cpu_of_numa_node = cpumask_first(cpumask);
>>
>>        if (first_cpu_of_numa_node >= nr_cpu_ids)
>>
>>              return -1;
>>
>> -#ifdef CONFIG_X86_64
>>
>> -     return cpu_data(first_cpu_of_numa_node).apicid;
>>
>> -#else
>>
>>        return first_cpu_of_numa_node;
>>
>> -#endif
>>
>> }
>>
>> /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first 
>> logical processor
>>
>> --
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 8801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] drm/amdkfd: Use logical cpu id for building vcrat

[Kuehling, Felix]

On 2019-04-16 2:44 a.m., Christian König wrote:
>>
>> It's not a high priority as I'm not aware of any applications that 
>> actually make use of the cache information.
>>
> Which raises the question why we have done this in the first place? 
> When nobody is using it could we just remove the interface?

The interface for cache information in the topology has existed since 
KFD was first introduced for APUs. On APUs the information is contained 
in the CRAT table. I see no reason not to report available information 
to user mode. If you wanted to remove it, you'd need to prove that 
nobody is using it. My statement was much weaker than that.

Furthermore, the cache information on CPUs is currently generated in 
user mode anyway. Changing the way we count CPU cores would break that 
existing user mode code, so we can't do that. That said, there is no 
kernel code to remove here. All I was saying was, that it's not a high 
priority to add the kernel code to populate CPU cache information in 
kernel mode.

Regards,
   Felix


>
> Regards,
> Christian.
>
> Am 16.04.19 um 05:24 schrieb Kuehling, Felix:
>>
>> On x86 we use the apicid to associate caches with CPU cores. See the 
>> Thunk code in libhsakmt/src/topology.c (static void 
>> find_cpu_cache_siblings()). If we used a different way to identify 
>> CPU cores, I think that would break. This code in the Thunk is 
>> x86-specific as it uses the CPUID instruction. We don't have 
>> equivalent code for ARM. So for ARM it doesn't really matter much, 
>> how you count your CPU cores in the CRAT table.
>>
>> I think eventually we want to get rid of that fragile CPUID code in 
>> the Thunk and get the cache information in kernel mode and report it 
>> to user mode through the KFD topology sysfs filesystem. Then we could 
>> also move away from using apicids as CPU IDs on x86.
>>
>> It's not a high priority as I'm not aware of any applications that 
>> actually make use of the cache information.
>>
>> Regards,
>>   Felix
>>
>> On 2019-04-15 22:39, Hillf Danton wrote:
>>>
>>> Hi folks
>>>
>>> In commit d1c234e2cd, arm64 is granted to build kfd. Currently, it 
>>> is physical
>>>
>>> cpu id that is used for building the x86_64 vcrat, but logical cpu 
>>> id is used
>>>
>>> instead for arm64, though the function name requires apicid. Can we 
>>> use the
>>>
>>> physical id for both arches if it really has an up-hand over the 
>>> logical one,
>>>
>>> as the following tiny diff represents?
>>>
>>> --- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
>>> 2019-04-16 07:55:56.611685400 +0800
>>>
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 
>>> 09:16:50.506126600 +0800
>>>
>>> @@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
>>>
>>> first_cpu_of_numa_node = cpumask_first(cpumask);
>>>
>>>        if (first_cpu_of_numa_node >= nr_cpu_ids)
>>>
>>>              return -1;
>>>
>>> -#ifdef CONFIG_X86_64
>>>
>>> -     return cpu_data(first_cpu_of_numa_node).apicid;
>>>
>>> -#else
>>>
>>> -     return first_cpu_of_numa_node;
>>>
>>> -#endif
>>>
>>> +    return cpu_physical_id(first_cpu_of_numa_node);
>>>
>>> }
>>>
>>> /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first 
>>> logical processor
>>>
>>> --
>>>
>>> Or is logical cpu id enough to do the work, with some cosmetic 
>>> applied to the
>>>
>>> function names(not included in the following simple diff yet)?
>>>
>>> thanks
>>>
>>> Hillf
>>>
>>> --- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 
>>> 2019-04-16 07:55:56.611685400 +0800
>>>
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c 2019-04-16 
>>> 09:18:24.546578400 +0800
>>>
>>> @@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
>>>
>>> first_cpu_of_numa_node = cpumask_first(cpumask);
>>>
>>>        if (first_cpu_of_numa_node >= nr_cpu_ids)
>>>
>>>              return -1;
>>>
>>> -#ifdef CONFIG_X86_64
>>>
>>> -     return cpu_data(first_cpu_of_numa_node).apicid;
>>>
>>> -#else
>>>
>>>        return first_cpu_of_numa_node;
>>>
>>> -#endif
>>>
>>> }
>>>
>>> /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first 
>>> logical processor
>>>
>>> --
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[RFC] drm/amdkfd: Use logical cpu id for building vcrat

[Hillf Danton]

[-- Attachment #1.1: Type: text/plain, Size: 1730 bytes --]

Hi folks

In commit d1c234e2cd, arm64 is granted to build kfd. Currently, it is physical
cpu id that is used for building the x86_64 vcrat, but logical cpu id is used
instead for arm64, though the function name requires apicid. Can we use the
physical id for both arches if it really has an up-hand over the logical one,
as the following tiny diff represents?

--- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c	2019-04-16 07:55:56.611685400 +0800
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c	2019-04-16 09:16:50.506126600 +0800
@@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
	first_cpu_of_numa_node = cpumask_first(cpumask);
	if (first_cpu_of_numa_node >= nr_cpu_ids)
		return -1;
-#ifdef CONFIG_X86_64
-	return cpu_data(first_cpu_of_numa_node).apicid;
-#else
-	return first_cpu_of_numa_node;
-#endif
+	return cpu_physical_id(first_cpu_of_numa_node);
}
 /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first logical processor
--


Or is logical cpu id enough to do the work, with some cosmetic applied to the 
function names(not included in the following simple diff yet)?

thanks
Hillf


--- linux-5.1-rc4/drivers/gpu/drm/amd/amdkfd/kfd_topology.c	2019-04-16 07:55:56.611685400 +0800
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c	2019-04-16 09:18:24.546578400 +0800
@@ -1405,11 +1405,7 @@ static int kfd_cpumask_to_apic_id(const
	first_cpu_of_numa_node = cpumask_first(cpumask);
	if (first_cpu_of_numa_node >= nr_cpu_ids)
		return -1;
-#ifdef CONFIG_X86_64
-	return cpu_data(first_cpu_of_numa_node).apicid;
-#else
	return first_cpu_of_numa_node;
-#endif
}
 /* kfd_numa_node_to_apic_id - Returns the APIC ID of the first logical processor
--


[-- Attachment #1.2: Type: text/html, Size: 5271 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx