diff mbox series

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

Message ID 20201209160956.32456-5-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: support per-cpupool scheduling granularity | expand

Commit Message

Jürgen Groß Dec. 9, 2020, 4:09 p.m. UTC 
Add a HYPFS_VARDIR_INIT() macro for initializing such a directory
statically, taking a struct hypfs_funcs pointer as parameter additional
to those of HYPFS_DIR_INIT().

Modify HYPFS_VARSIZE_INIT() to take the function vector pointer as an
additional parameter as this will be needed for dynamical entries.

For being able to let the generic hypfs coding continue to work on
normal struct hypfs_entry entities even for dynamical nodes add some
infrastructure for allocating a working area for the current hypfs
request in order to store needed information for traversing the tree.
This area is anchored in a percpu pointer and can be retrieved by any
level of the dynamic entries. The normal way to handle allocation and
freeing is to allocate the data in the enter() callback of a node and
to free it in the related exit() callback.

Add a hypfs_add_dyndir() function for adding a dynamic directory
template to the tree, which is needed for having the correct reference
to its position in hypfs.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- switch to xzalloc_bytes() in hypfs_alloc_dyndata() (Jan Beulich)
- carved out from previous patch
- use enter() and exit() callbacks for allocating and freeing
  dyndata memory
- add hypfs_add_dyndir()

V3:
- switch hypfs_alloc_dyndata() to be type safe (Jan Beulich)
- rename HYPFS_VARDIR_INIT() to HYPFS_DIR_INIT_FUNC() (Jan Beulich)

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c      | 31 +++++++++++++++++++++++++++++++
 xen/include/xen/hypfs.h | 29 +++++++++++++++++++----------
 2 files changed, 50 insertions(+), 10 deletions(-)

Comments

Jan Beulich Dec. 17, 2020, 11:01 a.m. UTC | #1 
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
Jürgen Groß Dec. 17, 2020, 11:24 a.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index f04934db10..8faf65cea0 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -72,6 +72,7 @@  enum hypfs_lock_state {
     hypfs_write_locked
 };
 static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked);
+static DEFINE_PER_CPU(struct hypfs_dyndata *, hypfs_dyndata);
 
 static DEFINE_PER_CPU(const struct hypfs_entry *, hypfs_last_node_entered);
 
@@ -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));
+}
+
 static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new)
 {
     int ret = -ENOENT;
@@ -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;
+}
+
 int hypfs_add_leaf(struct hypfs_entry_dir *parent,
                    struct hypfs_entry_leaf *leaf, bool nofault)
 {
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index a6dfdb7d8e..4c469cbeb4 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -76,7 +76,7 @@  struct hypfs_entry_dir {
     struct list_head dirlist;
 };
 
-#define HYPFS_DIR_INIT(var, nam)                  \
+#define HYPFS_DIR_INIT_FUNC(var, nam, fn)         \
     struct hypfs_entry_dir __read_mostly var = {  \
         .e.type = XEN_HYPFS_TYPE_DIR,             \
         .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
@@ -84,22 +84,25 @@  struct hypfs_entry_dir {
         .e.size = 0,                              \
         .e.max_size = 0,                          \
         .e.list = LIST_HEAD_INIT(var.e.list),     \
-        .e.funcs = &hypfs_dir_funcs,              \
+        .e.funcs = (fn),                          \
         .dirlist = LIST_HEAD_INIT(var.dirlist),   \
     }
 
-#define HYPFS_VARSIZE_INIT(var, typ, nam, msz)    \
-    struct hypfs_entry_leaf __read_mostly var = { \
-        .e.type = (typ),                          \
-        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
-        .e.name = (nam),                          \
-        .e.max_size = (msz),                      \
-        .e.funcs = &hypfs_leaf_ro_funcs,          \
+#define HYPFS_DIR_INIT(var, nam)                  \
+    HYPFS_DIR_INIT_FUNC(var, nam, &hypfs_dir_funcs)
+
+#define HYPFS_VARSIZE_INIT(var, typ, nam, msz, fn) \
+    struct hypfs_entry_leaf __read_mostly var = {  \
+        .e.type = (typ),                           \
+        .e.encoding = XEN_HYPFS_ENC_PLAIN,         \
+        .e.name = (nam),                           \
+        .e.max_size = (msz),                       \
+        .e.funcs = (fn),                           \
     }
 
 /* Content and size need to be set via hypfs_string_set_reference(). */
 #define HYPFS_STRING_INIT(var, nam)               \
-    HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0)
+    HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0, &hypfs_leaf_ro_funcs)
 
 /*
  * Set content and size of a XEN_HYPFS_TYPE_STRING node. The node will point
@@ -150,6 +153,8 @@  extern struct hypfs_entry_dir hypfs_root;
 
 int hypfs_add_dir(struct hypfs_entry_dir *parent,
                   struct hypfs_entry_dir *dir, bool nofault);
+void hypfs_add_dyndir(struct hypfs_entry_dir *parent,
+                      struct hypfs_entry_dir *template);
 int hypfs_add_leaf(struct hypfs_entry_dir *parent,
                    struct hypfs_entry_leaf *leaf, bool nofault);
 const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry);
@@ -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))
+void *hypfs_get_dyndata(void);
+void hypfs_free_dyndata(void);
 #endif
 
 #endif /* __XEN_HYPFS_H__ */