Skip to content

Commit

Permalink
Pull up following revision(s) (requested by riastradh in ticket #423):
Browse files Browse the repository at this point in the history
	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: rmind/thmap#14
PR kern/57666
rmind/thmap#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

rmind/npf#129
  • Loading branch information
MartinHusemann committed Oct 18, 2023
1 parent e063caf commit 7a5d101
Showing 1 changed file with 57 additions and 22 deletions.
79 changes: 57 additions & 22 deletions sys/kern/subr_thmap.c
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* $NetBSD: subr_thmap.c,v 1.12 2022/04/09 23:51:57 riastradh Exp $ */
/* $NetBSD: subr_thmap.c,v 1.12.4.1 2023/10/18 15:03:12 martin Exp $ */

/*-
* Copyright (c) 2018 Mindaugas Rasiukevicius <rmind at noxt eu>
Expand Down Expand Up @@ -112,7 +112,7 @@
#include "utils.h"
#endif

THMAP_RCSID("$NetBSD: subr_thmap.c,v 1.12 2022/04/09 23:51:57 riastradh Exp $");
THMAP_RCSID("$NetBSD: subr_thmap.c,v 1.12.4.1 2023/10/18 15:03:12 martin Exp $");

#include <crypto/blake2/blake2s.h>

Expand Down Expand Up @@ -212,11 +212,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)

Expand Down Expand Up @@ -252,6 +258,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.
*/
Expand Down Expand Up @@ -395,7 +429,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;
}
Expand Down Expand Up @@ -456,7 +490,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;
}
Expand All @@ -467,9 +501,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);
Expand All @@ -487,9 +521,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 *
Expand Down Expand Up @@ -547,7 +581,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 EEXIST;
}
/* Release to subsequent consume in find_edge_node(). */
Expand Down Expand Up @@ -927,11 +961,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
Expand All @@ -958,8 +994,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;
}
}
Expand Down Expand Up @@ -989,12 +1024,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);
}

Expand Down Expand Up @@ -1029,7 +1064,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));
}

0 comments on commit 7a5d101

Please sign in to comment.