[PATCH] 9pfs: cleanup V9fsFidState

[PATCH] 9pfs: cleanup V9fsFidState

[Christian Schoenebeck]

Drop V9fsFidState's 'next' member, which is no longer used since:

  f5265c8f917e ('9pfs: use GHashTable for fid table')

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index a6f59abccb..5e041e1f60 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -280,7 +280,6 @@ struct V9fsFidState {
     uid_t uid;
     int ref;
     bool clunked;
-    QSIMPLEQ_ENTRY(V9fsFidState) next;
     QSLIST_ENTRY(V9fsFidState) reclaim_next;
 };
 
-- 
2.39.5

Re: [PATCH] 9pfs: cleanup V9fsFidState

[Greg Kurz]

On Thu, 21 Nov 2024 11:52:48 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Drop V9fsFidState's 'next' member, which is no longer used since:
> 
>   f5265c8f917e ('9pfs: use GHashTable for fid table')
> 

Good catch !

Fixes: f5265c8f917e ('9pfs: use GHashTable for fid table')
Reviewed-by: Greg Kurz <groug@kaod.org>

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index a6f59abccb..5e041e1f60 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -280,7 +280,6 @@ struct V9fsFidState {
>      uid_t uid;
>      int ref;
>      bool clunked;
> -    QSIMPLEQ_ENTRY(V9fsFidState) next;
>      QSLIST_ENTRY(V9fsFidState) reclaim_next;
>  };
>  



-- 
Greg

Re: [PATCH] 9pfs: cleanup V9fsFidState

[Christian Schoenebeck]

On Thursday, November 21, 2024 12:07:39 PM CET Greg Kurz wrote:
> On Thu, 21 Nov 2024 11:52:48 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > Drop V9fsFidState's 'next' member, which is no longer used since:
> > 
> >   f5265c8f917e ('9pfs: use GHashTable for fid table')
> > 
> 
> Good catch !

Coincidence. I was reviewing & benchmarking this FID management code, because
sometimes hash maps are quite slow on iterating over all elements. So I made
a quick dual-container test with GHashTable for lookup of course, and while
using the old list container for 9p code parts that need to iterate over all
FIDs.

Fortunately it turned out though that iterating over all GHashTable elements
is even a tiny bit faster than iterating over the linked list, so GHashTable
is all we need, i.e. fast and simple code.

But ... I also learned that Linus' patch improved more than we expected.
Because just reverting these few lines:

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c7c433c06e..a5159ad766 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -330,6 +330,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
      * reclaim won't close the file descriptor
      */
     f->flags |= FID_REFERENCED;
+    QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
     g_hash_table_insert(s->fid_table, GINT_TO_POINTER(fid), f);
 
     v9fs_readdir_init(s->proto_version, &f->fs.dir);
@@ -420,6 +421,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
     /* TODO: Use g_hash_table_steal_extended() instead? */
     fidp = g_hash_table_lookup(s->fid_table, GINT_TO_POINTER(fid));
     if (fidp) {
+        QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
         g_hash_table_remove(s->fid_table, GINT_TO_POINTER(fid));
         fidp->clunked = true;
         return fidp;
@@ -4246,6 +4248,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     s->ctx.fmode = fse->fmode;
     s->ctx.dmode = fse->dmode;
 
+    QSIMPLEQ_INIT(&s->fid_list);
     s->fid_table = g_hash_table_new(NULL, NULL);
     qemu_co_rwlock_init(&s->rename_lock);
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index c92da2751a..177f4bab53 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -339,6 +339,7 @@ typedef struct {
 struct V9fsState {
     QLIST_HEAD(, V9fsPDU) free_list;
     QLIST_HEAD(, V9fsPDU) active_list;
+    QSIMPLEQ_HEAD(, V9fsFidState) fid_list;
     GHashTable *fid_table;
     FileOperations *ops;
     FsContext ctx;

Caused my test guest installation's performance to drop down to hell, i.e. I
could literally see and count the individual characters being updated on
guest's screen, that slow!

Which was a bit surprising first, because all it does is inserting and
removing elements to the old list. But then I realized, right, that's because
it was not a doubly linked list, but a simple linked list, so every
QSIMPLEQ_REMOVE() was actually traversing the list from head on.

So old and new conclusion: always start by checking your data structure types
first!

> Fixes: f5265c8f917e ('9pfs: use GHashTable for fid table')
> Reviewed-by: Greg Kurz <groug@kaod.org>

I'll add the fixes tag silently on my end.

Thanks!

/Christian