OS-6013 want more efficient id_space_t

[joyent-automation]

OS-6013 want more efficient id_space_t

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/2445/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@pfmooney commented at 2017-08-23T22:12:13

Patch Set 2:

(10 comments)

First group of stuff

@isaacdavis commented at 2017-08-24T16:19:59

Patch Set 2:

(5 comments)

@isaacdavis commented at 2017-08-25T00:47:19

Patch Set 2:

(4 comments)

Patch Set 2 code comments

usr/src/common/idspace/id_space.c#90 @pfmooney

These should definitely go.

usr/src/common/idspace/id_space.c#124 @pfmooney

Considering the kernel uses the nice CPU-specific functions, it'd probably be nice to do something similar for the userspace library. We could probably just make an 'extern' definition here and clone the appropriate asm implementations into the libidspace dir.

At the very least, this should probably be static.

usr/src/common/idspace/id_space.c#170 @pfmooney

Since these only have one caller each, maybe move the contents into their calling function?

usr/src/common/idspace/id_space.c#170 @isaacdavis

Done

usr/src/common/idspace/id_space.c#221 @pfmooney

bzero is more commonly used in the kernel space. (It also has one less param and doesn't require (void)-ing a return value)

usr/src/common/idspace/id_space.c#221 @isaacdavis

Done

usr/src/common/idspace/id_space.c#232 @pfmooney

The paranoid would use a uint_t for looping.

usr/src/common/idspace/id_space.c#232 @isaacdavis

Done

usr/src/common/idspace/id_space.c#247 @pfmooney

This is always called with an expectation that sleeping is allowed. The parameter should probably be removed.

usr/src/common/idspace/id_space.c#247 @isaacdavis

With the addition of id_space_create_nosleep, this is no longer true.

usr/src/common/idspace/id_space.c#292 @pfmooney

Why not make these direct returns? If not, why the negative number assigned to a uint?

usr/src/common/idspace/id_space.c#292 @isaacdavis

Done

usr/src/common/idspace/id_space.c#321 @pfmooney

Sleeping allocations should never fail. If you want to handle ENOMEM in userspace, perhaps an assert in the alloc/create function would be appropriate.

usr/src/common/idspace/id_space.c#321 @isaacdavis

I've restructured this a little bit so the null-checks only happen in userspace.

usr/src/common/idspace/id_space.c#354 @pfmooney

not possessive here, maybe 'id_tree_t entities'?

usr/src/common/idspace/id_space.c#354 @isaacdavis

Done

usr/src/common/idspace/id_space.c#397 @pfmooney

I love the ASCII visuals here

usr/src/common/idspace/id_space.c#397 @isaacdavis

Don't thank me, thank the writers of the weenix vmem subsystem!

usr/src/common/idspace/id_space.c#669 @isaacdavis

This comment is entirely wrong/misleading - please disregard.

@rmustacc commented at 2017-08-25T19:09:50

Patch Set 3:

(56 comments)

This looks like a great start. I've got a bunch of questions here on some of the design and some of the specifics. While there's a lot of comments, there's a lot of good stuff here.

Patch Set 3 code comments

usr/src/common/idspace/id_space.c#63 @rmustacc

Are you sure this is correct? I don't see how MAXUID is on the scene here at all. Also, the range should be marked as exclusive on the far end.

usr/src/common/idspace/id_space.c#64 @rmustacc

Is this still true?

usr/src/common/idspace/id_space.c#103 @rmustacc

What's the maximal ID?

usr/src/common/idspace/id_space.c#191 @rmustacc

I'm not sure what this notation is trying to convey.

usr/src/common/idspace/id_space.c#197 @rmustacc

Why is it rare?

usr/src/common/idspace/id_space.c#200 @rmustacc

Same comment on notation.

usr/src/common/idspace/id_space.c#246 @rmustacc

Don't use register.

usr/src/common/idspace/id_space.c#272 @rmustacc

Isn't this fls(3C)?

usr/src/common/idspace/id_space.c#279 @rmustacc

We should VERIFY that the idt_root is NULL.

usr/src/common/idspace/id_space.c#293 @rmustacc

Should we verify that there are no entries in the AVL list at this point?

usr/src/common/idspace/id_space.c#312 @rmustacc

Um. What are you trying to do here. Are you trying to satisfy the compiler? If so that really deserves a comment.

usr/src/common/idspace/id_space.c#318 @rmustacc

Why not just use the _zalloc version of the routines?

usr/src/common/idspace/id_space.c#335 @rmustacc

Is there a way to do this iteratively so we don't end up in a large recursive call? We don't have as much kernel stack.

I'd also recommend setting the entries to NULL as you go.

usr/src/common/idspace/id_space.c#354 @rmustacc

I would probably use zalloc here. While you're initializing most members, you're not doing anything with the AVL members for example. While it's unlikely that they'd be used uninitialized, I think it'll be safer to do something with them.

usr/src/common/idspace/id_space.c#356 @rmustacc

Same thing about compiler avoidance.

usr/src/common/idspace/id_space.c#384 @rmustacc

I would make sure to NULL out tp->idt_root before taking the next call.

usr/src/common/idspace/id_space.c#402 @rmustacc

This looks wrong. One of these should return -1, the other should return 1.

usr/src/common/idspace/id_space.c#421 @rmustacc

If these are invalid we should fail the creation in userland. It'd be great if we could get there for the kernel, but that probably needs more work.

usr/src/common/idspace/id_space.c#421 @rmustacc

I suspect the point of this is to make sure that we don't overflow the id_t with high. Given that it's defined to be an int through typedefs, shouldn't use something more appropriate here? Further, it looks like MAXUID is INT_MAX. Are you sure that adding one here won't underflow as it's not defined to be an unsigned type?

usr/src/common/idspace/id_space.c#424 @rmustacc

I'll continue my zalloc recommendations.

usr/src/common/idspace/id_space.c#451 @rmustacc

Why aren't you checking the return value?

usr/src/common/idspace/id_space.c#461 @rmustacc

Please create a single id_space_create function that takes a boolean_t for sleeping or not. We shouldn't duplicate all this. All of the previous comments apply to this.

usr/src/common/idspace/id_space.c#519 @rmustacc

VERIFY0.

usr/src/common/idspace/id_space.c#565 @rmustacc

See prior notes on this part of the ASSERT.

usr/src/common/idspace/id_space.c#641 @rmustacc

Use NBBY.

usr/src/common/idspace/id_space.c#652 @rmustacc

ASSERT3U

usr/src/common/idspace/id_space.c#654 @rmustacc

spaces around divide?

usr/src/common/idspace/id_space.c#665 @rmustacc

If you want all ffs in he value, please use the unsigned maximum value, not the signed negative value.

usr/src/common/idspace/id_space.c#678 @rmustacc

Are you sure this should be a signed value?

usr/src/common/idspace/id_space.c#708 @rmustacc

NBBY

usr/src/common/idspace/id_space.c#709 @rmustacc

unsigned

usr/src/common/idspace/id_space.c#769 @rmustacc

index?

usr/src/common/idspace/id_space.c#792 @rmustacc

Is there any way to design it to avoid the stack allocations?

usr/src/common/idspace/id_space.c#798 @rmustacc

Should we be asserting / verifying that we hold the tree's lock here?

usr/src/common/idspace/id_space.c#808 @rmustacc

Probably worth making that unsigned.

Also, we should probably ASSERT/VERIFY that it's non-zero so we don't get a divide by zero.

usr/src/common/idspace/id_space.c#808 @rmustacc

If ID_BRANCH_SHIFT * tree_levels is greater than 32, I suspect that might be undefined.

usr/src/common/idspace/id_space.c#827 @rmustacc

Is there any way for this to be negative?

usr/src/common/idspace/id_space.c#837 @rmustacc

Would you mind commenting why this case is a failure? I'm not 100% sure. You also don't need parens around it.

usr/src/common/idspace/id_space.c#843 @rmustacc

Mind asserting with a comparison?

usr/src/common/idspace/id_space.c#853 @rmustacc

What stops or bounds us to make sure we don't overflow the stack with recursive calls? I wouldn't assume we'll get a tail call.

usr/src/common/idspace/id_space.c#862 @rmustacc

Missing parens around return expression.

usr/src/common/idspace/id_space.c#873 @rmustacc

What guarantees that this never overflows?

usr/src/common/idspace/id_space.c#881 @rmustacc

Same question on overflow.

usr/src/common/idspace/id_space.c#913 @rmustacc

Rather than keeping track of the path we took, should we have parent pointers? This may also help in debugging. That way if we blow up on some random tree entry, we can always find the way to the parent.

usr/src/common/idspace/id_space.c#1010 @rmustacc

ASSERT3U, here and elsewhere.

usr/src/common/idspace/id_space.c#1169 @rmustacc

So, why aren't we checking the return value here? If we free an unallocated ID, that's bad, right?

usr/src/uts/common/sys/id_space.h#38 @rmustacc

I don't think you need to include <sys/mutex.h>. mutex(9F) and condvar(9F) only say you need sys/ksynch.h. Any reason you need include?

usr/src/uts/common/sys/id_space.h#41 @rmustacc

You want synch.h actually. thread.h includes it, but I don't think you want any of the thread specific stuff.

usr/src/uts/common/sys/id_space.h#44 @rmustacc

how did we come up with this number?

usr/src/uts/common/sys/id_space.h#45 @rmustacc

Is there some way that we can enforce this at compile time or otherwise? When you say sizeof (ulong_t) here, do you mean that the value must be larger than 4/8? I assume this is because of the division?

usr/src/uts/common/sys/id_space.h#47 @rmustacc

How did we come up with this number?

usr/src/uts/common/sys/id_space.h#48 @rmustacc

How did we come up with this number? It looks like it came from VMEM. But given that you're not using the following bytes, any reason not to round it up (the compiler is going to add padding anyways).

usr/src/uts/common/sys/id_space.h#51 @rmustacc

NBBY is the macro to use here. (Number of bits in a byte).

Also, did you look at the bitset.h or bitmap.h implementations at all? Is there a reason neither of them work for what we need to do?

usr/src/uts/common/sys/id_space.h#60 @rmustacc

It'd probably be useful to comment the other names as well.

usr/src/uts/common/sys/id_space.h#66 @rmustacc

Any particular reason we opted with an AVL over a list? It appears that we only ever walk the AVL tree by iterating over it like a list. If we're not using avl_find() maybe we should just use a list_t.

usr/src/uts/common/sys/id_space.h#77 @rmustacc

I would make sure to leave the actual types opaque to consumers and only define the id_space_t as an opaque structure and instead have an implementation specific header. This way the actual implementation doesn't leak into ddi compliant modules since it's designed to be opaque.