| Message ID | E1tE4v2-0051EH-Ni@kylie.crudebyte.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: cleanup V9fsFidState | expand |
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
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; };
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(-)