nvme: Use the per-namespace uma(9) zone for the bio.
Needs ReviewPublic
Actions

Authored by seigo.tanimura_gmail.com on May 27 2024, 10:08 AM.

Details

Reviewers

olce
kib
chuck
mav

Summary

The usage is essentially the same as the VM swap pager.

New Loader Tunable & Sysctl Entry

hw.nvme.nvme_reserved_new_bios The number of the bios reserved and preallocated per namespace. This is writable for hotplug.

Signed-off-by: Seigo Tanimura <seigo.tanimura@gmail.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57933
Build 54821: arc lint + arc unit

Event Timeline

seigo.tanimura_gmail.com created this revision.May 27 2024, 10:08 AM

Herald added subscribers: dab, imp. · View Herald TranscriptMay 27 2024, 10:08 AM

seigo.tanimura_gmail.com requested review of this revision.May 27 2024, 10:08 AM

Harbormaster completed remote builds in B57933: Diff 139135.May 27 2024, 10:08 AM

seigo.tanimura_gmail.com added a parent revision: D45215: kern/geom: Make the struct bio allocation more robust to a heavy load..May 27 2024, 10:09 AM

seigo.tanimura_gmail.com mentioned this in D45215: kern/geom: Make the struct bio allocation more robust to a heavy load..

olce added a reviewer: olce.May 27 2024, 1:09 PM

olce mentioned this in D45379: kern/vm: Use the dedicated bio zone for the swap bio..May 27 2024, 1:11 PM

imp added reviewers: kib, chuck, mav.May 27 2024, 2:36 PM

I'd like to see data that shows this is the hot path. I recently added counters to count the splits.
Ideally, it would use whatever cloning zones already exist and not invent a new one for the nvme drive, imho.
Do you get the same, better or worse performance if you just disable this feature of the nvme drive entirely?

share/man/man4/nda.4
70	This is the wrong man page to document this.
sys/dev/nvme/nvme_ns.c
387	This style is wrong. Don't use NULL != foo or NULL == foo in the kernel. Usee foo != NULL. Also, if it's cloning, it should be using the zone for the bio preferentially. This is exactly cloning, but with different parameters. Finally, this seems as good a point as any, does this actually get called? The cloning code was for some really only drives originally, and more modern ones don't seem to have parameters that make it worthwhile anymore.
599	Style: don't use backwards compares.
600	Don't use backwards compares. (here and everywhere)
sys/dev/nvme/nvme_private.h
103	This shouldn't be a global.
sys/dev/nvme/nvme_sysctl.c
44	This shouldn't be a global. This should be a per-ns tunable (or at least per nvme drive since we don't have per-ns sysctls at the moment). with a default value.
55	Again, not global, but per controller.

I am not fully sure about the motivation of this change, but It feels wrong to me to have per-namespace zones. On a big system under heavy load UMA does a lot of work for per-CPU and per-domain caching, and doing it also per-namespace would multiply resource waste. Also last time I touched it, I remember it was difficult for UMA to operate in severely constrained environments, since eviction of per-CPU caches is quite expensive. I don't remember how reservation works in that context, but I suppose that having dozens of small zones with small reservations, but huge per-CPU caches is not a very viable configuration.

netchild added a subscriber: netchild.Jul 15 2024, 6:28 AM