Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

thmap_create tests wrong value for allocation failure #13

Closed
riastradh opened this issue Oct 17, 2023 · 0 comments · Fixed by #14
Closed

thmap_create tests wrong value for allocation failure #13

riastradh opened this issue Oct 17, 2023 · 0 comments · Fixed by #14

Comments

@riastradh
Copy link
Contributor

On allocation failure, the alloc operation returns zero, but thmap_create tests whether the THMAP_GETPTR-relocated pointer is null instead:

thmap/src/thmap.c

Lines 896 to 898 in 8149b27

root = thmap->ops->alloc(THMAP_ROOT_LEN);
thmap->root = THMAP_GETPTR(thmap, root);
if (!thmap->root) {

This is in contrast to all the other allocation failure tests, such as:

thmap/src/thmap.c

Lines 312 to 315 in 8149b27

p = thmap->ops->alloc(THMAP_INODE_LEN);
if (!p) {
return NULL;
}

However, t_thmap.c allocates starting from zero. So it can't actually represent failure:

thmap/src/t_thmap.c

Lines 19 to 20 in 8149b27

static unsigned space_allocated = 0;
static unsigned char space[42500];

thmap/src/t_thmap.c

Lines 240 to 245 in 8149b27

alloc_test_wrapper(size_t len)
{
uintptr_t p = space_allocated;
space_allocated += roundup2(len, sizeof(void *));
assert(space_allocated <= sizeof(space));
return p;

riastradh pushed a commit to riastradh/thmap that referenced this issue Oct 17, 2023
thmap_create: Test allocation, not relocation, for null.
39d9bf3 
Adapt test so it never returns zero on success.

fix rmind#13
@riastradh riastradh mentioned this issue Oct 17, 2023
Merged
netbsd-srcmastr pushed a commit to NetBSD/src that referenced this issue Oct 17, 2023
thmap(9): Test alloc failure, not THMAP_GETPTR failure.
272918d 
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

XXX pullup-10
XXX pullup-9
netbsd-srcmastr pushed a commit to NetBSD/src that referenced this issue Oct 20, 2023
@MartinHusemann
Pull up following revision(s) (requested by riastradh in ticket #1755):
10ff328 
	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
netbsd-srcmastr pushed a commit to NetBSD/src that referenced this issue Oct 20, 2023
@MartinHusemann
Pull up following revision(s) (requested by riastradh in ticket #423):
7a5d101 
	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
rokuyama pushed a commit to IIJ-NetBSD/netbsd-src that referenced this issue Oct 26, 2023
thmap(9): Test alloc failure, not THMAP_GETPTR failure.
4c943d4 
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

XXX pullup-10
XXX pullup-9
rokuyama pushed a commit to IIJ-NetBSD/netbsd-src that referenced this issue Oct 26, 2023
@MartinHusemann
Pull up following revision(s) (requested by riastradh in ticket #423):
7691cfc 
	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
rokuyama pushed a commit to IIJ-NetBSD/netbsd-src that referenced this issue Oct 27, 2023
@MartinHusemann
Pull up following revision(s) (requested by riastradh in ticket #1755):
dd47b3c 
	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
@rmind rmind closed this as completed in #14 Oct 30, 2023
rmind pushed a commit that referenced this issue Oct 30, 2023
@riastradh
thmap_create: Test allocation, not relocation, for null. (#14)
87063f9 
Adapt test so it never returns zero on success. Fixes #13.

Co-authored-by: Taylor R Campbell <campbell+rmind-thmap@mumble.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

thmap_create: Test allocation, not relocation, for null. riastradh/thmap
1 participant
@riastradh