[PATCH v3] drm/amdkfd: Use dynamic allocation for CU occupancy array in 'kfd_get_cu_occupancy()'

[Joshi, Mukul]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>
> Sent: Tuesday, October 8, 2024 11:43 PM
> To: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM at amd.com>; Joshi, Mukul <Mukul.Joshi at amd.com>;
> Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; Kuehling, Felix
> <Felix.Kuehling at amd.com>
> 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 at amd.com>
> Cc: Harish Kasiviswanathan <Harish.Kasiviswanathan at amd.com>
> Cc: Felix Kuehling <felix.kuehling at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
> 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