Peter Xu <peterx@xxxxxxxxxx> writes:
> On Tue, Sep 10, 2024 at 11:43:57PM +0000, Ackerley Tng wrote:
>> @@ -1079,12 +1152,20 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
>> if (err)
>> goto out;
>>
>> + err = -ENOMEM;
>> + private = kzalloc(sizeof(*private), GFP_KERNEL);
>> + if (!private)
>> + goto out;
>> +
>> if (flags & KVM_GUEST_MEMFD_HUGETLB) {
>> - err = kvm_gmem_hugetlb_setup(inode, size, flags);
>> + err = kvm_gmem_hugetlb_setup(inode, private, size, flags);
>> if (err)
>> - goto out;
>> + goto free_private;
>> }
>>
>> + xa_init(&private->faultability);
>> + inode->i_mapping->i_private_data = private;
>> +
>> inode->i_private = (void *)(unsigned long)flags;
>
> Looks like inode->i_private isn't used before this series; the flags was
> always zero before anyway. Maybe it could keep kvm_gmem_inode_private
> instead? Then make the flags be part of the struct.
>
> It avoids two separate places (inode->i_mapping->i_private_data,
> inode->i_private) to store gmem private info.
>
Weakly-held opinion: I think the advantage of re-using inode->i_private
to store flags is that in some cases, e.g. non-hugetlb, we might be able
to avoid an allocation (of kvm_gmem_inode_private).
Does anyone else have any thoughts on this?
>> inode->i_op = &kvm_gmem_iops;
>> inode->i_mapping->a_ops = &kvm_gmem_aops;
>> @@ -1097,6 +1178,8 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
>>
>> return inode;
>>
>> +free_private:
>> + kfree(private);
>> out:
>> iput(inode);
>>
>> --
>> 2.46.0.598.g6f2099f65c-goog
>>
>
> --
> Peter Xu
