On 17.12.20 12:01, Jan Beulich wrote:
> On 09.12.2020 17:09, Juergen Gross wrote:
>> @@ -158,6 +159,30 @@ static void node_exit_all(void)
>>           node_exit(*last);
>>   }
>>   
>> +void *hypfs_alloc_dyndata_size(unsigned long size)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +
>> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
>> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
>> +
>> +    per_cpu(hypfs_dyndata, cpu) = xzalloc_bytes(size);
>> +
>> +    return per_cpu(hypfs_dyndata, cpu);
>> +}
>> +
>> +void *hypfs_get_dyndata(void)
>> +{
>> +    ASSERT(this_cpu(hypfs_dyndata));
>> +
>> +    return this_cpu(hypfs_dyndata);
>> +}
>> +
>> +void hypfs_free_dyndata(void)
>> +{
>> +    XFREE(this_cpu(hypfs_dyndata));
>> +}
> 
> In all three cases, would an intermediate local variable perhaps
> yield better generated code? (In hypfs_get_dyndata() this may be
> less important because the 2nd use is only an ASSERT().)

Okay.

> 
>> @@ -219,6 +244,12 @@ int hypfs_add_dir(struct hypfs_entry_dir *parent,
>>       return ret;
>>   }
>>   
>> +void hypfs_add_dyndir(struct hypfs_entry_dir *parent,
>> +                      struct hypfs_entry_dir *template)
>> +{
>> +    template->e.parent = &parent->e;
>> +}
> 
> I'm struggling with the direction here: This makes the template
> point at the parent, but the parent will still have no
> "knowledge" of its new templated children. I suppose that's how
> it is meant to be, but maybe this could do with a comment, since
> it's the opposite way of hypfs_add_dir()?

I'll add a comment.

> 
> Also - does this mean parent may not also have further children,
> templated or "normal"?

No, the related read and findentry functions just need to cover that
case, e.g. by calling multiple sub-functions.

> 
>> @@ -177,6 +182,10 @@ struct hypfs_entry *hypfs_leaf_findentry(const struct hypfs_entry_dir *dir,
>>   struct hypfs_entry *hypfs_dir_findentry(const struct hypfs_entry_dir *dir,
>>                                           const char *name,
>>                                           unsigned int name_len);
>> +void *hypfs_alloc_dyndata_size(unsigned long size);
>> +#define hypfs_alloc_dyndata(type) (type *)hypfs_alloc_dyndata_size(sizeof(type))
> 
> This wants an extra pair of parentheses.

Okay.

> 
> As a minor point, I also wonder whether you really want the type
> unsafe version to be easily usable. It would be possible to
> largely "hide" it by having
> 
> void *hypfs_alloc_dyndata(unsigned long size);
> #define hypfs_alloc_dyndata(type) ((type *)hypfs_alloc_dyndata(sizeof(type)))

Yes, will change.


Juergen