From dd47b3c92a6d37d63cddb1e412a8b692c55c522f Mon Sep 17 00:00:00 2001 From: martin Date: Wed, 18 Oct 2023 15:07:06 +0000 Subject: [PATCH] Pull up following revision(s) (requested by riastradh in ticket #1755): sys/kern/subr_thmap.c: revision 1.14 sys/kern/subr_thmap.c: revision 1.15 thmap(9): Test alloc failure, not THMAP_GETPTR failure. THMAP_GETPTR may return nonnull even though alloc returned zero. Note that this failure branch is not actually appropriate; thmap_create should not fail. We really need to pass KM_SLEEP through in this call site even though there are other call sites for which KM_NOSLEEP is appropriate. Adapted from: https://github.com/rmind/thmap/pull/14 PR kern/57666 https://github.com/rmind/thmap/issues/13 thmap(9): Preallocate GC list storage for thmap_del. thmap_del can't fail, and it is used in places in npf where sleeping is forbidden, so it can't rely on allocating memory either. Instead of having thmap_del allocate memory on the fly for each object to defer freeing until thmap_gc, arrange to have thmap(9) preallocate the same storage when allocating all the objects in the first place, with a GC header. This is suboptimal for memory usage, especially on insertion- and lookup-heavy but deletion-light workloads, but it's not clear rmind's alternative (https://github.com/rmind/thmap/tree/thmap_del_mem_fail) is ready to use yet, so we'll go with this for correctness. PR kern/57208 https://github.com/rmind/npf/issues/129 --- sys/kern/subr_thmap.c | 79 +++++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 22 deletions(-) diff --git a/sys/kern/subr_thmap.c b/sys/kern/subr_thmap.c index 06c583010cea..e73d875e4194 100644 --- a/sys/kern/subr_thmap.c +++ b/sys/kern/subr_thmap.c @@ -1,4 +1,4 @@ -/* $NetBSD: subr_thmap.c,v 1.5.6.1 2020/05/25 17:19:37 martin Exp $ */ +/* $NetBSD: subr_thmap.c,v 1.5.6.2 2023/10/18 15:07:06 martin Exp $ */ /*- * Copyright (c) 2018 Mindaugas Rasiukevicius @@ -111,7 +111,7 @@ #include "utils.h" #endif -THMAP_RCSID("$NetBSD: subr_thmap.c,v 1.5.6.1 2020/05/25 17:19:37 martin Exp $"); +THMAP_RCSID("$NetBSD: subr_thmap.c,v 1.5.6.2 2023/10/18 15:07:06 martin Exp $"); /* * NetBSD kernel wrappers @@ -207,11 +207,17 @@ typedef struct { uint32_t hashval; // current hash value } thmap_query_t; -typedef struct { - uintptr_t addr; +union thmap_align { + void * p; + uint64_t v; +}; + +typedef struct thmap_gc thmap_gc_t; +struct thmap_gc { size_t len; - void * next; -} thmap_gc_t; + thmap_gc_t * next; + char data[] __aligned(sizeof(union thmap_align)); +}; #define THMAP_ROOT_LEN (sizeof(thmap_ptr_t) * ROOT_SIZE) @@ -246,6 +252,34 @@ static const thmap_ops_t thmap_default_ops = { .free = free_wrapper }; +static uintptr_t +gc_alloc(const thmap_t *thmap, size_t len) +{ + const size_t alloclen = offsetof(struct thmap_gc, data[len]); + const uintptr_t gcaddr = thmap->ops->alloc(alloclen); + + if (!gcaddr) + return 0; + + thmap_gc_t *const gc = THMAP_GETPTR(thmap, gcaddr); + gc->len = len; + return THMAP_GETOFF(thmap, &gc->data[0]); +} + +static void +gc_free(const thmap_t *thmap, uintptr_t addr, size_t len) +{ + const size_t alloclen = offsetof(struct thmap_gc, data[len]); + char *const ptr = THMAP_GETPTR(thmap, addr); + thmap_gc_t *const gc = container_of(ptr, struct thmap_gc, data[0]); + const uintptr_t gcaddr = THMAP_GETOFF(thmap, gc); + + KASSERTMSG(gc->len == len, "thmap=%p ops=%p addr=%p len=%zu" + " gc=%p gc->len=%zu", + thmap, thmap->ops, (void *)addr, len, gc, gc->len); + thmap->ops->free(gcaddr, alloclen); +} + /* * NODE LOCKING. */ @@ -361,7 +395,7 @@ node_create(thmap_t *thmap, thmap_inode_t *parent) thmap_inode_t *node; uintptr_t p; - p = thmap->ops->alloc(THMAP_INODE_LEN); + p = gc_alloc(thmap, THMAP_INODE_LEN); if (!p) { return NULL; } @@ -422,7 +456,7 @@ leaf_create(const thmap_t *thmap, const void *key, size_t len, void *val) thmap_leaf_t *leaf; uintptr_t leaf_off, key_off; - leaf_off = thmap->ops->alloc(sizeof(thmap_leaf_t)); + leaf_off = gc_alloc(thmap, sizeof(thmap_leaf_t)); if (!leaf_off) { return NULL; } @@ -433,9 +467,9 @@ leaf_create(const thmap_t *thmap, const void *key, size_t len, void *val) /* * Copy the key. */ - key_off = thmap->ops->alloc(len); + key_off = gc_alloc(thmap, len); if (!key_off) { - thmap->ops->free(leaf_off, sizeof(thmap_leaf_t)); + gc_free(thmap, leaf_off, sizeof(thmap_leaf_t)); return NULL; } memcpy(THMAP_GETPTR(thmap, key_off), key, len); @@ -453,9 +487,9 @@ static void leaf_free(const thmap_t *thmap, thmap_leaf_t *leaf) { if ((thmap->flags & THMAP_NOCOPY) == 0) { - thmap->ops->free(leaf->key, leaf->len); + gc_free(thmap, leaf->key, leaf->len); } - thmap->ops->free(THMAP_GETOFF(thmap, leaf), sizeof(thmap_leaf_t)); + gc_free(thmap, THMAP_GETOFF(thmap, leaf), sizeof(thmap_leaf_t)); } static thmap_leaf_t * @@ -510,7 +544,7 @@ root_try_put(thmap_t *thmap, const thmap_query_t *query, thmap_leaf_t *leaf) nptr = THMAP_GETOFF(thmap, node); again: if (atomic_load_relaxed(&thmap->root[i])) { - thmap->ops->free(nptr, THMAP_INODE_LEN); + gc_free(thmap, nptr, THMAP_INODE_LEN); return false; } /* Release to subsequent consume in find_edge_node(). */ @@ -883,11 +917,13 @@ thmap_del(thmap_t *thmap, const void *key, size_t len) static void stage_mem_gc(thmap_t *thmap, uintptr_t addr, size_t len) { + char *const ptr = THMAP_GETPTR(thmap, addr); thmap_gc_t *head, *gc; - gc = kmem_intr_alloc(sizeof(thmap_gc_t), KM_NOSLEEP); - gc->addr = addr; - gc->len = len; + gc = container_of(ptr, struct thmap_gc, data[0]); + KASSERTMSG(gc->len == len, + "thmap=%p ops=%p ptr=%p len=%zu gc=%p gc->len=%zu", + thmap, thmap->ops, (char *)addr, len, gc, gc->len); retry: head = atomic_load_relaxed(&thmap->gc_list); gc->next = head; // not yet published @@ -914,8 +950,7 @@ thmap_gc(thmap_t *thmap, void *ref) while (gc) { thmap_gc_t *next = gc->next; - thmap->ops->free(gc->addr, gc->len); - kmem_intr_free(gc, sizeof(thmap_gc_t)); + gc_free(thmap, THMAP_GETOFF(thmap, &gc->data[0]), gc->len); gc = next; } } @@ -945,12 +980,12 @@ thmap_create(uintptr_t baseptr, const thmap_ops_t *ops, unsigned flags) if ((thmap->flags & THMAP_SETROOT) == 0) { /* Allocate the root level. */ - root = thmap->ops->alloc(THMAP_ROOT_LEN); - thmap->root = THMAP_GETPTR(thmap, root); - if (!thmap->root) { + root = gc_alloc(thmap, THMAP_ROOT_LEN); + if (!root) { kmem_free(thmap, sizeof(thmap_t)); return NULL; } + thmap->root = THMAP_GETPTR(thmap, root); memset(thmap->root, 0, THMAP_ROOT_LEN); atomic_thread_fence(memory_order_release); /* XXX */ } @@ -984,7 +1019,7 @@ thmap_destroy(thmap_t *thmap) thmap_gc(thmap, ref); if ((thmap->flags & THMAP_SETROOT) == 0) { - thmap->ops->free(root, THMAP_ROOT_LEN); + gc_free(thmap, root, THMAP_ROOT_LEN); } kmem_free(thmap, sizeof(thmap_t)); }