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

[Jürgen Groß <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature