On 3/4/2025 1:49 PM, Felix Kuehling wrote:
> On 2025-02-21 4:23, Zhu Lingshan wrote:
>> This commit initialized svm lists at where they are
>> defined. This is defensive programing for security
>> and consistency.
>>
>> Initalizing variables ensures that their states are
>> always valid, avoiding issues caused by
>> uninitialized variables that could lead to
>> unpredictable behaviros.
> The lists are clearly documented as output parameters in the svm_range_add
> function definition. I think it's more readable to do the list initialization
> in svm_range_add to keep the logic of the caller more readable. One
> suggestion inline that would move the initialization to the caller without
> cluttering the calling function's code.
>
>
>> And we should not assume the callee would always
>> initialize them
>>
>> Signed-off-by: Zhu Lingshan <lingshan....@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index bd3e20d981e0..cbc997449379 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -2130,11 +2130,7 @@ svm_range_add(struct kfd_process *p, uint64_t start,
>> uint64_t size,
>>
>> pr_debug("svms 0x%p [0x%llx 0x%lx]\n", &p->svms, start, last);
>>
>> - INIT_LIST_HEAD(update_list);
>> - INIT_LIST_HEAD(insert_list);
>> - INIT_LIST_HEAD(remove_list);
>> INIT_LIST_HEAD(&new_list);
>> - INIT_LIST_HEAD(remap_list);
>>
>> node = interval_tree_iter_first(&svms->objects, start, last);
>> while (node) {
>> @@ -3635,6 +3631,11 @@ svm_range_set_attr(struct kfd_process *p, struct
>> mm_struct *mm,
>> if (r)
>> return r;
>>
>> + INIT_LIST_HEAD(&update_list);
>> + INIT_LIST_HEAD(&insert_list);
>> + INIT_LIST_HEAD(&remove_list);
>> + INIT_LIST_HEAD(&remap_list);
>> +
> You could initialize these where they are defined by replacing the struct
> list_head ... definitions with
>
> LIST_HEAD(update_list);
> LIST_HEAD(insert_list);
Yes, this is better, I will use LIST_HEAD and remove the initialization in
svm_range_add because we don't need to init them twice
By the way, I am not sure the lists are "output parameters", usually an output
parameter should carry some information for other functions,
but here the lists are just defined, even not initialized, and are on the
stack. Actually the callee only fills the lists and the caller itself use
the lists. I suggest to remove the "output parameters" in the code comments.
Thanks
Lingshan
> ...
>
> Regards,
> Felix
>
>
>> svms = &p->svms;
>>
>> mutex_lock(&process_info->lock);
