[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>
> Sent: Tuesday, October 8, 2024 11:43 PM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@xxxxxxx>; Joshi, Mukul <Mukul.Joshi@xxxxxxx>;
> Kasiviswanathan, Harish <Harish.Kasiviswanathan@xxxxxxx>; Kuehling, Felix
> <Felix.Kuehling@xxxxxxx>
> Subject: [PATCH v3] drm/amdkfd: Use dynamic allocation for CU occupancy array in
> 'kfd_get_cu_occupancy()'
>
> The `kfd_get_cu_occupancy` function previously declared a large `cu_occupancy`
> array as a local variable, which could lead to stack overflows due to excessive stack
> usage. This commit replaces the static array allocation with dynamic memory
> allocation using `kcalloc`, thereby reducing the stack size.
>
> This change avoids the risk of stack overflows in kernel space, in scenarios where
> `AMDGPU_MAX_QUEUES` is large. The allocated memory is freed using `kfree`
> before the function returns to prevent memory leaks.
>
> Fixes the below with gcc W=1:
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c: In function
> ‘kfd_get_cu_occupancy’:
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c:322:1: warning: the frame size
> of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 322 | }
> | ^
>
> Fixes: 6fc91266b798 ("drm/amdkfd: Update logic for CU occupancy calculations")
> Suggested-by: Mukul Joshi <mukul.joshi@xxxxxxx>
> Cc: Harish Kasiviswanathan <Harish.Kasiviswanathan@xxxxxxx>
> Cc: Felix Kuehling <felix.kuehling@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
> v3:
> - Remove unused "bytes_written" (Mukul)
> fixes the below:
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c: In function
> ‘kfd_get_cu_occupancy’:
> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_process.c:276:13: warning: unused
> variable ‘bytes_written’ [-Wunused-variable]
> 276 | int bytes_written;
> | ^~~~~~~~~~~~~
>
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index d665ecdcd12f..45fe75078b73 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -271,10 +271,12 @@ static int kfd_get_cu_occupancy(struct attribute *attr, char
> *buffer)
> struct kfd_process *proc = NULL;
> struct kfd_process_device *pdd = NULL;
> int i;
> - struct kfd_cu_occupancy cu_occupancy[AMDGPU_MAX_QUEUES];
> + struct kfd_cu_occupancy *cu_occupancy;
> u32 queue_format;
>
> - memset(cu_occupancy, 0x0, sizeof(cu_occupancy));
> + cu_occupancy = kcalloc(AMDGPU_MAX_QUEUES, sizeof(*cu_occupancy),
> GFP_KERNEL);
> + if (!cu_occupancy)
> + return -ENOMEM;
>
You need to free this memory for other return statements in the function, for example:
if (dev->kfd2kgd->get_cu_occupancy == NULL)
return -EINVAL;
and another "if (pdd->qpd.queue_count == 0)", otherwise you will leak memory.
I would suggest moving the kcalloc call just before we do wave_cnt = 0.
Regards,
Mukul
> pdd = container_of(attr, struct kfd_process_device, attr_cu_occupancy);
> dev = pdd->dev;
> @@ -318,6 +320,7 @@ static int kfd_get_cu_occupancy(struct attribute *attr, char
> *buffer)
>
> /* Translate wave count to number of compute units */
> cu_cnt = (wave_cnt + (max_waves_per_cu - 1)) / max_waves_per_cu;
> + kfree(cu_occupancy);
> return snprintf(buffer, PAGE_SIZE, "%d\n", cu_cnt); }
>
> --
> 2.34.1
