This assertion should go at the beginning of the function.

What's the point of deferring the unlock? We hold a critical section here and there's a free slot in uc_freebucket, so we're guaranteed to succeed during the restart. In all other cases we are jumping back with locked == false, right?

"an upper limit" would sound more natural to me, and you use that term below.

We want the zone lock here, not the keg lock.

Why did this get inlined? I can't see the reason.

The extra locking is rather unfortunate. Why do we do the check here and not somewhere where we already hold the zone lock?

Since you did it elsewhere, get rid of this return statement too?

"This zone's keg"

But existing uma_zone_set_max() comment already has this wording.

In the new scheme if zone has a keg, they share lock.

The zone_timeout() is called for only keg-enabled zones, see zone_foreach(). So, KEG_LOCK() here is correct and is self-documenting.

I'm preparing a patch to put it back into separate function, and to be fair, I feel that it is more difficult to understand the code when a middle of uma_zalloc_arg() is separated from the rest of the function.

If you really insist I can roll this back. However, zone_alloc_bucket() would be modified. You see, the inlined code doesn't exactly match code of the function. It has few additions to rollback uz_items in case of allocation failure or strange bucket.

Because uz_import is not always zone_import(). For vm_page zones uz_import it is outside of UMA and doesn't lock zone. Right before uz_import is the only point where we can do bookkeeping of allocations.

The performance impact here is not measurable. We are already at a slow path and are about to acquire some kind of lock in uz_import. For regular zones, the same lock, so same cache line. Also, ratio of fastpath allocs to slowpath is expected to be super low. For example for vm_page we got one slowpath alloc for 10^7 immediate returns from per-CPU cache with zero jumps to zalloc_start.

Shouldn't we be adding bucket->ub_cnt to uz_frees?

I don't understand this check. In the common case the bucket will be placed in the per-CPU cache, so it won't count against uz_bktcount. And we already check this condition again before adding it to the bucket list.

Should assert that zone->uz_items >= max - bucket->ub_cnt before the subtraction.

I still think I prefer having a separate function. The details of how we fill a bucket aren't really relevant to the core allocation algorithm. At least, please pick a more specific variable name than "max".

What problem does this line fix?

It is weird to me that the patch is being so careful in general to do precise accounting, but then just adjusts the caller's request here. Why not simply disable per-CPU caching in this case?

The parens are not needed anymore.

Why?

You are absolutely correct. This came from Netflix tree and later my profiling showed that condition never happens.

Without per-CPU caching UMA isn't a useful allocator at all.

Note that uma_zone_set_max() never claimed to put a hard cap on a zone. With new code we are getting closer to requested cap than we used before.

So that we may use single pointer for uz_keg/uz_lockptr. Should I put that in comments?

And yet UMA_NOBUCKET exists. :)

I pointed this out because you are using UMA zones to manage pbufs, and the existing allocator is precise. For certain uses, where the max is a function of the about of physical memory, it may be that we don't care about per-CPU caching at all on small systems.

Right, I see now. A comment would be helpful, I think.

UMA_NOBUCKET doesn't turn off CPU caches.

The existing allocator is precise, but maximums provided to it are very very rough guesses. "Mkay, 1/2 nswbufs might not be enough, but 3/4 would be fine..." For pbufs potential adjustment done by uma_zone_set_max() is entirely okay. I even see it as an autotuning. The more CPUs machine has, the more buffers it would have.

Will do.

Are you thinking of UMA_NOBUCKETCACHE? That is a different flag (and I'm glad it's gone with this change). It is a bug if UMA_NOBUCKET _doesn't_ turn off per-CPU caches.

Ah, sorry. Yes, I meant UMA_NOBUCKETCACHE.

I prefer an explicit comparison: if (zone->uz_maxitems > 0)

Why do we duplicate the logic from zone_alloc_item() instead of simply jumping to zone_alloc_item()?

msleep(9) is deprecated by mtx_sleep(9).

Explicit comparison: zone->uz_sleepers > 0.

Why do you not do a wakeup here?

Consider adding a KEG_LOCK_ASSERT()?

Explicit comparison: uz_sleepers > 0.

I don't believe there's any padding here.

To be consistent this should be uz_max_items.

Again, I'd prefer consistency in the names: uz_bkt_count, uz_bkt_max.

Seems to be a dup?

Why not have it in zone_alloc_item_locked()? With the current diff, uma_large_malloc(), for example, can bypass limits.

For me your suggestion looks more obfuscated rather then clearer.

This is now used for a single pointer and can be removed.

Did you verify this offset?

Yes, I noticed that. At Netflix we already have that substituted for an other field. I will address that later.

I did. Will verify once again pre-commit.