[PATCH 1/4] drm/amdkfd: Create node in kfd sysfs for memory only numa node

[PATCH 1/4] drm/amdkfd: Create node in kfd sysfs for memory only numa node

[Oak Zeng]

Previously kfd driver assumes all CPU numa nodes are associated
with system memory and there is no memory only numa node. This
is not true anymore for ALDEBARAN A+A set up. This patch creates
memory only node in kfd sysfs.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 73 ++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 3251fe2..56e6dff 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -827,8 +827,11 @@ static int kfd_parse_subtype_mem(struct crat_subtype_memory *mem,
 	uint32_t flags = 0;
 	uint32_t width;
 
-	pr_debug("Found memory entry in CRAT table with proximity_domain=%d\n",
-			mem->proximity_domain);
+	size_in_bytes =
+		((uint64_t)mem->length_high << 32) +
+		mem->length_low;
+	pr_debug("Found memory entry in CRAT table with proximity_domain=%d, size %lld\n",
+			mem->proximity_domain, size_in_bytes);
 	list_for_each_entry(dev, device_list, list) {
 		if (mem->proximity_domain == dev->proximity_domain) {
 			/* We're on GPU node */
@@ -848,9 +851,6 @@ static int kfd_parse_subtype_mem(struct crat_subtype_memory *mem,
 			if (mem->flags & CRAT_MEM_FLAGS_NON_VOLATILE)
 				flags |= HSA_MEM_FLAGS_NON_VOLATILE;
 
-			size_in_bytes =
-				((uint64_t)mem->length_high << 32) +
-							mem->length_low;
 			width = mem->width;
 
 			/* Multiple banks of the same type are aggregated into
@@ -1718,51 +1718,62 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size)
 	sub_type_hdr = (struct crat_subtype_generic *)(crat_table+1);
 
 	for_each_online_node(numa_node_id) {
+		pr_debug("numa node id %d\n", numa_node_id);
 		if (kfd_numa_node_to_apic_id(numa_node_id) == -1)
-			continue;
-
-		/* Fill in Subtype: Compute Unit */
-		ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size,
-			crat_table->num_domains,
-			(struct crat_subtype_computeunit *)sub_type_hdr);
-		if (ret < 0)
-			return ret;
-		crat_table->length += sub_type_hdr->length;
-		crat_table->total_entries++;
+			pr_debug("Numa node %d is a memory only numa node\n", numa_node_id);
+
+		if (kfd_numa_node_to_apic_id(numa_node_id) != -1) {
+			/* Fill in Subtype: Compute Unit */
+			ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size,
+					crat_table->num_domains,
+					(struct crat_subtype_computeunit *)sub_type_hdr);
+			if (ret < 0) {
+				pr_err("fill cu for cpu failed\n");
+				return ret;
+			}
+			crat_table->length += sub_type_hdr->length;
+			crat_table->total_entries++;
 
-		sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
-			sub_type_hdr->length);
+			sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
+					sub_type_hdr->length);
+		}
 
 		/* Fill in Subtype: Memory */
 		ret = kfd_fill_mem_info_for_cpu(numa_node_id, &avail_size,
 			crat_table->num_domains,
 			(struct crat_subtype_memory *)sub_type_hdr);
-		if (ret < 0)
+		if (ret < 0) {
+			pr_err("fill mem for cpu failed\n");
 			return ret;
+		}
 		crat_table->length += sub_type_hdr->length;
 		crat_table->total_entries++;
 
 		sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
 			sub_type_hdr->length);
 
-		/* Fill in Subtype: IO Link */
+		if (kfd_numa_node_to_apic_id(numa_node_id) != -1) {
+			/* Fill in Subtype: IO Link */
 #ifdef CONFIG_X86_64
-		ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size,
-				&entries,
-				(struct crat_subtype_iolink *)sub_type_hdr);
-		if (ret < 0)
-			return ret;
+			ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size,
+					&entries,
+					(struct crat_subtype_iolink *)sub_type_hdr);
+			if (ret < 0) {
+				pr_err("fill iolink for cpu failed\n");
+				return ret;
+			}
 
-		if (entries) {
-			crat_table->length += (sub_type_hdr->length * entries);
-			crat_table->total_entries += entries;
+			if (entries) {
+				crat_table->length += (sub_type_hdr->length * entries);
+				crat_table->total_entries += entries;
 
-			sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
-					sub_type_hdr->length * entries);
-		}
+				sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
+						sub_type_hdr->length * entries);
+			}
 #else
-		pr_info("IO link not available for non x86 platforms\n");
+			pr_info("IO link not available for non x86 platforms\n");
 #endif
+		}
 
 		crat_table->num_domains++;
 	}
-- 
2.7.4

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

[PATCH 2/4] drm/amdkfd: Don't create crat memory sub-table if node has no memory

[Oak Zeng]

In some configuration, there is CPU-only (no memory) numa node. Don't
create crat memory sub-table for such node.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 56e6dff..dd7772c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -1583,7 +1583,8 @@ static int kfd_fill_cu_for_cpu(int numa_node_id, int *avail_size,
  *	@avail_size: Available size in the memory
  *	@sub_type_hdr: Memory into which compute info will be filled in
  *
- *	Return 0 if successful else return -ve value
+ *	Return memory size in bytes if successful else return -ve value
+ *	Returning 0 means this numa node has no memory
  */
 static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size,
 			int proximity_domain,
@@ -1619,7 +1620,7 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size,
 	sub_type_hdr->length_high = upper_32_bits(mem_in_bytes);
 	sub_type_hdr->proximity_domain = proximity_domain;
 
-	return 0;
+	return mem_in_bytes;
 }
 
 #ifdef CONFIG_X86_64
@@ -1746,11 +1747,15 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size)
 			pr_err("fill mem for cpu failed\n");
 			return ret;
 		}
-		crat_table->length += sub_type_hdr->length;
-		crat_table->total_entries++;
 
-		sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
-			sub_type_hdr->length);
+		/* ret == 0: this node has no memory */
+		if (ret > 0) {
+			crat_table->length += sub_type_hdr->length;
+			crat_table->total_entries++;
+
+			sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
+					sub_type_hdr->length);
+		}
 
 		if (kfd_numa_node_to_apic_id(numa_node_id) != -1) {
 			/* Fill in Subtype: IO Link */
-- 
2.7.4

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

[PATCH 3/4] drm/amdkfd: Use simd_count to determine whether it is a GPU node

[Oak Zeng]

Previously we used cpu_cores_count==0 to determine whether a node
is a GPU node. This is not correct any more since we introduced
memory only numa node. For memory only numa node, cpu_cores_count
is also 0 but it is not a GPU node.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index dd7772c..87226d5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -835,7 +835,7 @@ static int kfd_parse_subtype_mem(struct crat_subtype_memory *mem,
 	list_for_each_entry(dev, device_list, list) {
 		if (mem->proximity_domain == dev->proximity_domain) {
 			/* We're on GPU node */
-			if (dev->node_props.cpu_cores_count == 0) {
+			if (dev->node_props.simd_count != 0) {
 				/* APU */
 				if (mem->visibility_type == 0)
 					heap_type =
-- 
2.7.4

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

[PATCH 4/4] drm/amdkfd: Patch memory parameters for all CPU nodes

[Oak Zeng]

There can be more than one CPU nodes. Patch memory width
and clock info for all CPU nodes, instead of only the first
one.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
index 9be66ba..e982829 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
@@ -1073,10 +1073,10 @@ int kfd_topology_init(void)
 	if (cpu_only_node) {
 		/* Add additional information to CPU only node created above */
 		down_write(&topology_lock);
-		kdev = list_first_entry(&topology_device_list,
-				struct kfd_topology_device, list);
+		list_for_each_entry(kdev, &topology_device_list, list) {
+			kfd_add_non_crat_information(kdev);
+		}
 		up_write(&topology_lock);
-		kfd_add_non_crat_information(kdev);
 	}
 
 err:
-- 
2.7.4

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

Re: [PATCH 1/4] drm/amdkfd: Create node in kfd sysfs for memory only numa node

[Felix Kuehling]

Am 2021-06-01 um 3:07 p.m. schrieb Oak Zeng:
> Previously kfd driver assumes all CPU numa nodes are associated
> with system memory

This is not true. Systems where there is no memory installed in one or
more CPU sockets have been supported. The only thing that was not
supported is NUMA domains with memory but no CPU (or GPU).


>  and there is no memory only numa node. This
> is not true anymore for ALDEBARAN A+A set up. This patch creates
> memory only node in kfd sysfs.
>
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 73 ++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 3251fe2..56e6dff 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -827,8 +827,11 @@ static int kfd_parse_subtype_mem(struct crat_subtype_memory *mem,
>  	uint32_t flags = 0;
>  	uint32_t width;
>  
> -	pr_debug("Found memory entry in CRAT table with proximity_domain=%d\n",
> -			mem->proximity_domain);
> +	size_in_bytes =
> +		((uint64_t)mem->length_high << 32) +
> +		mem->length_low;
> +	pr_debug("Found memory entry in CRAT table with proximity_domain=%d, size %lld\n",
> +			mem->proximity_domain, size_in_bytes);
>  	list_for_each_entry(dev, device_list, list) {
>  		if (mem->proximity_domain == dev->proximity_domain) {
>  			/* We're on GPU node */
> @@ -848,9 +851,6 @@ static int kfd_parse_subtype_mem(struct crat_subtype_memory *mem,
>  			if (mem->flags & CRAT_MEM_FLAGS_NON_VOLATILE)
>  				flags |= HSA_MEM_FLAGS_NON_VOLATILE;
>  
> -			size_in_bytes =
> -				((uint64_t)mem->length_high << 32) +
> -							mem->length_low;
>  			width = mem->width;
>  
>  			/* Multiple banks of the same type are aggregated into
> @@ -1718,51 +1718,62 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size)
>  	sub_type_hdr = (struct crat_subtype_generic *)(crat_table+1);
>  
>  	for_each_online_node(numa_node_id) {
> +		pr_debug("numa node id %d\n", numa_node_id);
>  		if (kfd_numa_node_to_apic_id(numa_node_id) == -1)
> -			continue;
> -
> -		/* Fill in Subtype: Compute Unit */
> -		ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size,
> -			crat_table->num_domains,
> -			(struct crat_subtype_computeunit *)sub_type_hdr);
> -		if (ret < 0)
> -			return ret;
> -		crat_table->length += sub_type_hdr->length;
> -		crat_table->total_entries++;
> +			pr_debug("Numa node %d is a memory only numa node\n", numa_node_id);
> +
> +		if (kfd_numa_node_to_apic_id(numa_node_id) != -1) {

This should be an else-branch of the previous "if".


> +			/* Fill in Subtype: Compute Unit */
> +			ret = kfd_fill_cu_for_cpu(numa_node_id, &avail_size,
> +					crat_table->num_domains,
> +					(struct crat_subtype_computeunit *)sub_type_hdr);
> +			if (ret < 0) {
> +				pr_err("fill cu for cpu failed\n");
> +				return ret;
> +			}
> +			crat_table->length += sub_type_hdr->length;
> +			crat_table->total_entries++;
>  
> -		sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
> -			sub_type_hdr->length);
> +			sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
> +					sub_type_hdr->length);
> +		}
>  
>  		/* Fill in Subtype: Memory */
>  		ret = kfd_fill_mem_info_for_cpu(numa_node_id, &avail_size,
>  			crat_table->num_domains,

So you're creating a dangling memory object that's not associated with
any node and therefore has no IO links? I'm not sure how that will work.
I was expecting that there would be at least a dummy CPU node with 0
cores but with IO links to express the NUMA distances.

Regards,
  Felix


>  			(struct crat_subtype_memory *)sub_type_hdr);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			pr_err("fill mem for cpu failed\n");
>  			return ret;
> +		}
>  		crat_table->length += sub_type_hdr->length;
>  		crat_table->total_entries++;
>  
>  		sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
>  			sub_type_hdr->length);
>  
> -		/* Fill in Subtype: IO Link */
> +		if (kfd_numa_node_to_apic_id(numa_node_id) != -1) {
> +			/* Fill in Subtype: IO Link */
>  #ifdef CONFIG_X86_64
> -		ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size,
> -				&entries,
> -				(struct crat_subtype_iolink *)sub_type_hdr);
> -		if (ret < 0)
> -			return ret;
> +			ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size,
> +					&entries,
> +					(struct crat_subtype_iolink *)sub_type_hdr);
> +			if (ret < 0) {
> +				pr_err("fill iolink for cpu failed\n");
> +				return ret;
> +			}
>  
> -		if (entries) {
> -			crat_table->length += (sub_type_hdr->length * entries);
> -			crat_table->total_entries += entries;
> +			if (entries) {
> +				crat_table->length += (sub_type_hdr->length * entries);
> +				crat_table->total_entries += entries;
>  
> -			sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
> -					sub_type_hdr->length * entries);
> -		}
> +				sub_type_hdr = (typeof(sub_type_hdr))((char *)sub_type_hdr +
> +						sub_type_hdr->length * entries);
> +			}
>  #else
> -		pr_info("IO link not available for non x86 platforms\n");
> +			pr_info("IO link not available for non x86 platforms\n");
>  #endif
> +		}
>  
>  		crat_table->num_domains++;
>  	}
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx