[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH 08/12] xen/hypfs: support dynamic hypfs nodes



On 17.11.20 13:37, Jan Beulich wrote:
On 26.10.2020 10:13, Juergen Gross wrote:
Add a getsize() function pointer to struct hypfs_funcs for being able
to have dynamically filled entries without the need to take the hypfs
lock each time the contents are being generated.

But a dynamic update causing a change in size will require _some_
lock, won't it?

Yes, of course.

e.g. the getsize() function returning the size of a directory holding an
entry for each cpupool will need to take the cpupool lock in order to
avoid a cpupool being created or deleted in parallel.

But the cpupool create/destroy functions don't need to take the hypfs
lock.


--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -19,28 +19,29 @@
  CHECK_hypfs_dirlistentry;
  #endif
-#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
-#define DIRENTRY_SIZE(name_len) \
-    (DIRENTRY_NAME_OFF +        \
-     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
-
  struct hypfs_funcs hypfs_dir_funcs = {
      .read = hypfs_read_dir,
+    .getsize = hypfs_getsize,
+    .findentry = hypfs_dir_findentry,
  };
  struct hypfs_funcs hypfs_leaf_ro_funcs = {
      .read = hypfs_read_leaf,
+    .getsize = hypfs_getsize,
  };
  struct hypfs_funcs hypfs_leaf_wr_funcs = {
      .read = hypfs_read_leaf,
      .write = hypfs_write_leaf,
+    .getsize = hypfs_getsize,
  };
  struct hypfs_funcs hypfs_bool_wr_funcs = {
      .read = hypfs_read_leaf,
      .write = hypfs_write_bool,
+    .getsize = hypfs_getsize,
  };
  struct hypfs_funcs hypfs_custom_wr_funcs = {
      .read = hypfs_read_leaf,
      .write = hypfs_write_custom,
+    .getsize = hypfs_getsize,
  };

With the increasing number of fields that may (deliberately or
by mistake) be NULL, should we gain some form of proactive
guarding against calls through such pointers?

Hmm, up to now I think such a bug would be detected rather fast.

I can add some ASSERT()s for mandatory functions not being NULL when
a node is added dynamically or during hypfs initialization for the
static nodes.


@@ -88,6 +93,23 @@ static void hypfs_unlock(void)
      }
  }
+void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)

Will callers really need to specify (high) alignment values? IOW ...

+{
+    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(size, align);

... is xzalloc_bytes() not suitable for use here?

Good question.

Up to now I think we could get away without specific alignment.

I can drop that parameter for now if you'd like that better.


@@ -171,15 +193,34 @@ static int hypfs_get_path_user(char *buf,
      return 0;
  }
+struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
+                                        const char *name,
+                                        unsigned int name_len)
+{
+    struct hypfs_entry *entry;
+
+    list_for_each_entry ( entry, &dir->dirlist, list )
+    {
+        int cmp = strncmp(name, entry->name, name_len);
+
+        if ( cmp < 0 )
+            return ERR_PTR(-ENOENT);
+
+        if ( !cmp && strlen(entry->name) == name_len )
+            return entry;
+    }
+
+    return ERR_PTR(-ENOENT);
+}
+
  static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
                                                 const char *path)
  {
      const char *end;
      struct hypfs_entry *entry;
      unsigned int name_len;
-    bool again = true;
- while ( again )
+    for ( ;; )

Nit: Strictly speaking another blank is needed between the two
semicolons.

Okay.


@@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
l = container_of(entry, const struct hypfs_entry_leaf, e); - return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
+    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
+                                              -EFAULT : 0;

With the intended avoiding of locking, how is this ->getsize()
guaranteed to not ...

@@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
          goto out;
ret = -ENOBUFS;
-    if ( ulen < entry->size + sizeof(e) )
+    if ( ulen < size + sizeof(e) )
          goto out;

... invalidate the checking done here? (A similar risk looks to
exist on the write path, albeit there we have at least the
->max_size checks, where I hope that field isn't mean to become
dynamic as well.)

I think you are right. I should add the size value as a parameter to the
read and write functions.

And no, max_size should not be dynamic.


Juergen

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.