Partial refactoring of uma_zalloc_arg().
ClosedPublic
Actions

Authored by jeff on Nov 20 2019, 11:04 PM.

Details

Reviewers

kib
markj
glebius
rlibby

Commits

rS355121: Refactor uma_zalloc_arg(). It is a mess of gotos and code which doesn't

Summary

uma_zalloc_arg/zfree_arg have grown out of control. I would like to add another new feature and it may require a custom trip through uma_zalloc and I didn't want to add a bunch of new flag checks.

I attempted to make more of a fastpath without a total hack. This should dramatically reduce the amount of text involved in normal allocations. I moved some of the zitem checks closer to the allocation so they are more encapsulated. I still feel like a fair bit of this should be abstracted into inlines for clarity.

This diff should be functionally identical. I don't think it's perfect but I think it's a step in the right direction. I will be posting another for uma_zfree_arg() soon.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jeff created this revision.Nov 20 2019, 11:04 PM

Harbormaster completed remote builds in B27669: Diff 64651.Nov 20 2019, 11:04 PM

jeff edited the summary of this revision. (Show Details)Nov 20 2019, 11:09 PM

jeff added reviewers: kib, markj, glebius.

jeff set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptNov 20 2019, 11:09 PM

markj accepted this revision.Nov 21 2019, 8:54 PM

markj added inline comments.

sys/vm/uma_core.c
2542 ↗	(On Diff #64651)	Missing parens around the return value.
2551 ↗	(On Diff #64651)	Add a CRITICAL_ASSERT()?
2585 ↗	(On Diff #64651)	Make lockfail a bool while you're here?
2631 ↗	(On Diff #64651)	"let's"
3080 ↗	(On Diff #64651)	The fragment <update uz_items> if (zone->uz_sleepers > 0 && zone->uz_items < zone->uz_max_items) wakeup_one(zone); is pasted all over the place. This might be a good opportunity to fix that.

This revision is now accepted and ready to land.Nov 21 2019, 8:54 PM

Address review feedback.

This revision now requires review to proceed.Nov 21 2019, 10:40 PM

Harbormaster completed remote builds in B27699: Diff 64700.Nov 21 2019, 10:40 PM

markj accepted this revision.Nov 22 2019, 3:35 PM

This revision is now accepted and ready to land.Nov 22 2019, 3:35 PM

jeff added a reviewer: rlibby.Nov 23 2019, 7:39 PM

First pass, just looked at stuff related to item_ctor.

I wish I'd committed D20722 a couple months ago, although it will be simpler to rewrite now with this as base.

sys/vm/uma_core.c
2449–2451 ↗	(On Diff #64700)	Prediction is wrong, assuming you mean to predict no failure, and not no constructor.
2452–2454 ↗	(On Diff #64700)	This is nicer here... I left it out in D20722 because the three call sites have three different responses to error. It would be nice to unify them.
3076–3078 ↗	(On Diff #64700)	This now double counts the failure (once in `item_ctor()`, once under `fail:`).

I am not going to object to you committing yours first and me rebasing on top of that.

In D22470#492498, @jeff wrote:

I am not going to object to you committing yours first and me rebasing on top of that.

Okay. I will fix up the procedure signatures to match yours and update right now.

rlibby mentioned this in D20722: uma: trash memory when ctor/dtor supplied too.Nov 23 2019, 11:38 PM

I am going to commit this soon if there are no objections.

sys/vm/uma_core.c
2449–2451 ↗	(On Diff #64700)	Constructors are rare except on debug builds.
3080 ↗	(On Diff #64651)	I think it should be fixed but I will do that in a follow-up commit.