Re: [PATCH v3 4/8] xen/hypfs: support dynamic hypfs nodes

[Jan Beulich <jbeulich@xxxxxxxx>]

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().)

> @@ -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()?

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

> @@ -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.

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)))

Jan