Re: [PATCH] drm/i915: change node clearing from memset to initialization

[Tom Rix <trix@redhat.com>]

On 4/16/22 11:33 AM, Joe Perches wrote:
> On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
>> In insert_mappable_node(), the parameter node is
>> cleared late in node's use with memset.
>> insert_mappable_node() is a singleton, called only
>> from i915_gem_gtt_prepare() which itself is only
>> called by i915_gem_gtt_pread() and
>> i915_gem_gtt_pwrite_fast() where the definition of
>> node originates.
>>
>> Instead of using memset, initialize node to 0 at it's
>> definitions.
> trivia: /it's/its/
>
> Only reason _not_ to do this is memset is guaranteed to
> zero any padding that might go to userspace.
>
> But it doesn't seem there is any padding anyway nor is
> the struct available to userspace.
>
> So this seems fine though it might increase overall code
> size a tiny bit.
>
> I do have a caveat: see below:
>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> []
>> @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct drm_i915_gem_object *obj,
>>   		goto err_ww;
>>   	} else if (!IS_ERR(vma)) {
>>   		node->start = i915_ggtt_offset(vma);
>> -		node->flags = 0;
> Why is this unneeded?

node = {} initializes all of node's elements to 0, including this one.

Tom

>
> from: drm_mm_insert_node_in_range which can set node->flags
>
> 		__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
>
>