kenv: avoid sleepable alloc for integer tunables

[jah]

Summary

Avoid performing a potentially-blocking malloc for kenv lookups that will only
perform non-destructive integer conversions on the returned buffer. Instead,
perform the strtoq() in-place with the kenv lock held.

While here, factor the logic around kenv_lock acquire and release into
kenv_acquire() and kenv_release(), and use these functions for some light
cleanup. Collapse getenv_string_buffer() into kern_getenv(), as the former
no longer has any other callers and the only additional task performed by
the latter is a WITNESS check that hasn't been useful since r362231.

PR: 248250

[mjg]

Is kenv frequently used to begin with? rm lock would add memory use per-cpu, so arguably it would be a pessimization. I also suspect we may be able to get away without any locks to begin with with some trickery.

Either way, I think the change to strtok in place is a good idea and an equivalent should be done for other routines if applicable.

That said, can you please open a separate review for the locking change and only keep the actual bugfix here?

[jah]

In D26010#576843, @mjg wrote:

Is kenv frequently used to begin with? rm lock would add memory use per-cpu, so arguably it would be a pessimization. I also suspect we may be able to get away without any locks to begin with with some trickery.

Either way, I think the change to strtok in place is a good idea and an equivalent should be done for other routines if applicable.

That said, can you please open a separate review for the locking change and only keep the actual bugfix here?

Sure, I can do that.

I thought that this might be a candidate for UMA SMR with a bit more work, but I think we'd need to make careful use of barriered atomics in places like kern_[un]setenv() and _getenv_string(). At first thought, using that instead of rmlock for something like kenv seemed like overkill. rmlock also seemed like a better alternative (for fast-path reads) to the overhead of rwlock.

Would the per-cpu pessimization only apply to the slow path?

[mjg]

The statement was that rmlocks distribute the lock per-CPU which means increased memory use. There are cases where this is perfectly warranted of course, I just doubt this one qualifies.

kenv should almost never be used past boot and even then I highly doubt the lock is contended enough to warrant any non-trivial changes. There may be offenders which keep re-reading the same value when they could do it once, but I don't think they are worth tracking down either. I do suspect there are nice wins to get like avoiding memory alloc (which also happen to fix bugs), but past that I would suggest not messing with the area.

In my opinion justifying this particular conversion would require a real world workload running into the problem which happens to not abuse the interface (like aforementioned re-reads of the same stuff), but I doubt there is something legitimate like this out there. However, I'm not going to stand in the way.

If you are itching to do some SMP I'm happy to point you at things like pseudofs/procfs which need a lot of love.

[jah]

In D26010#576845, @mjg wrote:

The statement was that rmlocks distribute the lock per-CPU which means increased memory use. There are cases where this is perfectly warranted of course, I just doubt this one qualifies.

kenv should almost never be used past boot and even then I highly doubt the lock is contended enough to warrant any non-trivial changes. There may be offenders which keep re-reading the same value when they could do it once, but I don't think they are worth tracking down either. I do suspect there are nice wins to get like avoiding memory alloc (which also happen to fix bugs), but past that I would suggest not messing with the area.

In my opinion justifying this particular conversion would require a real world workload running into the problem which happens to not abuse the interface (like aforementioned re-reads of the same stuff), but I doubt there is something legitimate like this out there. However, I'm not going to stand in the way.

I think that's a fair assessment, I wondered the same thing when I wrote the patch. I'll return it back to a mutex.

If you are itching to do some SMP I'm happy to point you at things like pseudofs/procfs which need a lot of love.

Sure, I'd be interested in that. I don't have a huge amount of time for FreeBSD work, but I'm glad to help as long as it's not something that anyone needs tomorrow.

[jah]

Return kenv_lock to a mutex

[mjg]

I would s/kern_environment/kenv in the commit.

This can go in, but I would apply the suggestion I mentioned.

sys/kern/kern_environment.c

692

This should be folded into a helper similar to what getenv_string_buffer is right now.

Then this and the other caller would be:

buf = getenv_condlock(name);
if (buf == NULL) /* nothing to do here */
         return 0;
[work]
putenv_condlock(buf);

[jah]

Factor locking/unlocking into kenv_acquire() and kenv_release(), and use them for some light cleanup

[mjg]

Nice cleanup. This is an ACK from me conditional on the PR submitter retesting the change.