Re: kern/57208: panic related to NPF functionality

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/57208; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: he%NetBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost, rmind%NetBSD.org@localhost
Subject: Re: kern/57208: panic related to NPF functionality
Date: Thu, 12 Oct 2023 19:34:57 +0000

 This is a multi-part message in MIME format.
 --=_2Yq8a6fRuEaGoxA1LAInCnTY5Q7059bP
 
 The attached patch trades some extra memory usage in thmap(9) for
 avoiding the panic by preallocating the thmap_gc_t list entry data
 structures in each object they might be used to deferred-free.
 
 I'd like to pull this up into netbsd-10, even if we end up using a
 different approach later, whatever that approach is -- nobody's
 suggested anything better in the nine months since this was filed, and
 this should work by eliminating the possibility of failure in
 thmap_del without adding any new sleeps for allocation that might be
 hit in the softint packet-processing path.
 
 I ran npftest via the atf tests and it passed, but can you give it a
 try just to make sure it hasn't horribly broken anything else?
 
 --=_2Yq8a6fRuEaGoxA1LAInCnTY5Q7059bP
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57208-thmapgcprealloc"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57208-thmapgcprealloc.patch"
 
 From f5367bc5d1c34562fe381ce184ac4693da9b6a95 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Thu, 12 Oct 2023 14:53:45 +0000
 Subject: [PATCH] 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 nobody's proposed a
 better approach since the issue was reported, so we'll go with this
 for correctness.
 
 PR kern/57208
 https://github.com/rmind/npf/issues/129
 
 XXX pullup-10
 XXX pullup-9
 ---
  sys/kern/subr_thmap.c | 71 ++++++++++++++++++++++++++++++++-----------
  1 file changed, 53 insertions(+), 18 deletions(-)
 
 diff --git a/sys/kern/subr_thmap.c b/sys/kern/subr_thmap.c
 index 7af1dc33d0af..a0a8cee3ce62 100644
 --- a/sys/kern/subr_thmap.c
 +++ b/sys/kern/subr_thmap.c
 @@ -212,11 +212,17 @@ typedef struct {
  	uint32_t	hashval;	// current hash value
  } thmap_query_t;
 =20
 -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));
 +};
 =20
  #define	THMAP_ROOT_LEN	(sizeof(thmap_ptr_t) * ROOT_SIZE)
 =20
 @@ -252,6 +258,34 @@ static const thmap_ops_t thmap_default_ops =3D {
  	.free =3D free_wrapper
  };
 =20
 +static uintptr_t
 +gc_alloc(const thmap_t *thmap, size_t len)
 +{
 +	const size_t alloclen =3D offsetof(struct thmap_gc, data[len]);
 +	const uintptr_t gcaddr =3D thmap->ops->alloc(alloclen);
 +
 +	if (!gcaddr)
 +		return 0;
 +
 +	thmap_gc_t *const gc =3D THMAP_GETPTR(thmap, gcaddr);
 +	gc->len =3D 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 =3D offsetof(struct thmap_gc, data[len]);
 +	char *const ptr =3D THMAP_GETPTR(thmap, addr);
 +	thmap_gc_t *const gc =3D container_of(ptr, struct thmap_gc, data[0]);
 +	const uintptr_t gcaddr =3D THMAP_GETOFF(thmap, gc);
 +
 +	KASSERTMSG(gc->len =3D=3D len, "thmap=3D%p ops=3D%p addr=3D%p len=3D%zu"
 +	    " gc=3D%p gc->len=3D%zu",
 +	    thmap, thmap->ops, (void *)addr, len, gc, gc->len);
 +	thmap->ops->free(gcaddr, alloclen);
 +}
 +
  /*
   * NODE LOCKING.
   */
 @@ -395,7 +429,7 @@ node_create(thmap_t *thmap, thmap_inode_t *parent)
  	thmap_inode_t *node;
  	uintptr_t p;
 =20
 -	p =3D thmap->ops->alloc(THMAP_INODE_LEN);
 +	p =3D gc_alloc(thmap, THMAP_INODE_LEN);
  	if (!p) {
  		return NULL;
  	}
 @@ -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;
 =20
 -	leaf_off =3D thmap->ops->alloc(sizeof(thmap_leaf_t));
 +	leaf_off =3D gc_alloc(thmap, sizeof(thmap_leaf_t));
  	if (!leaf_off) {
  		return NULL;
  	}
 @@ -467,9 +501,9 @@ leaf_create(const thmap_t *thmap, const void *key, size=
 _t len, void *val)
  		/*
  		 * Copy the key.
  		 */
 -		key_off =3D thmap->ops->alloc(len);
 +		key_off =3D 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);
 @@ -487,9 +521,9 @@ static void
  leaf_free(const thmap_t *thmap, thmap_leaf_t *leaf)
  {
  	if ((thmap->flags & THMAP_NOCOPY) =3D=3D 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));
  }
 =20
  static thmap_leaf_t *
 @@ -547,7 +581,7 @@ root_try_put(thmap_t *thmap, const thmap_query_t *query=
 , thmap_leaf_t *leaf)
  	nptr =3D 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(). */
 @@ -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 =3D THMAP_GETPTR(thmap, addr);
  	thmap_gc_t *head, *gc;
 =20
 -	gc =3D kmem_intr_alloc(sizeof(thmap_gc_t), KM_NOSLEEP);
 -	gc->addr =3D addr;
 -	gc->len =3D len;
 +	gc =3D container_of(ptr, struct thmap_gc, data[0]);
 +	KASSERTMSG(gc->len =3D=3D len,
 +	    "thmap=3D%p ops=3D%p ptr=3D%p len=3D%zu gc=3D%p gc->len=3D%zu",
 +	    thmap, thmap->ops, (char *)addr, len, gc, gc->len);
  retry:
  	head =3D atomic_load_relaxed(&thmap->gc_list);
  	gc->next =3D head; // not yet published
 @@ -958,8 +994,7 @@ thmap_gc(thmap_t *thmap, void *ref)
 =20
  	while (gc) {
  		thmap_gc_t *next =3D 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 =3D next;
  	}
  }
 @@ -986,7 +1021,7 @@ thmap_create(uintptr_t baseptr, const thmap_ops_t *ops=
 , unsigned flags)
 =20
  	if ((thmap->flags & THMAP_SETROOT) =3D=3D 0) {
  		/* Allocate the root level. */
 -		root =3D thmap->ops->alloc(THMAP_ROOT_LEN);
 +		root =3D gc_alloc(thmap, THMAP_ROOT_LEN);
  		thmap->root =3D THMAP_GETPTR(thmap, root);
  		if (!thmap->root) {
  			kmem_free(thmap, sizeof(thmap_t));
 @@ -1026,7 +1061,7 @@ thmap_destroy(thmap_t *thmap)
  	thmap_gc(thmap, ref);
 =20
  	if ((thmap->flags & THMAP_SETROOT) =3D=3D 0) {
 -		thmap->ops->free(root, THMAP_ROOT_LEN);
 +		gc_free(thmap, root, THMAP_ROOT_LEN);
  	}
  	kmem_free(thmap, sizeof(thmap_t));
  }
 
 --=_2Yq8a6fRuEaGoxA1LAInCnTY5Q7059bP--