vfs: convert struct mount counters to per-cpu
ClosedPublic
Actions

Authored by mjg on Sep 13 2019, 4:28 PM.

Details

Reviewers

kib
jeff

Commits

rS352427: vfs: convert struct mount counters to per-cpu

Summary

A 32-bit zone is used to avoid deficiencies of the counter(9) api. It may be int is too small now and we should switch it at least on amd64, i.e. change it from int to long (keeping the smaller size on 32-bit platforms).

There are very few helpers here and the code is full of ugly int-casts. Perhaps this can be augmented.

These issues aside I consider the patch committable. Proposed commit message:

struct mount counters are very frequently updated, but their exact value is rarely needed. At the same time constant use of a centralized counter causes avoidable cacheline traffic. Avoid the problem by adding per-cpu variants for the common case. The code falls back to centralized counters in case of mnt_vfs_ops > 0.

Sample result of a 104-way fstatfs (performs busy/unbusy):
before: 852393 ops/s
after: 76682077 ops/s

Test Plan

Tested by me on powerpc64 and by pho.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mjg created this revision.Sep 13 2019, 4:28 PM

Herald added a subscriber: imp. · View Herald TranscriptSep 13 2019, 4:28 PM

Harbormaster completed remote builds in B26465: Diff 62038.Sep 13 2019, 4:28 PM

mjg added parent revisions: D21425: vfs: manage mnt_ref with atomics, D21574: vfs: manage mnt_lockref with atomics, D21575: vfs: manage mnt_writeopcount with atomics.Sep 13 2019, 4:32 PM

rebase on top of D21425

mjg edited the summary of this revision. (Show Details)Sep 14 2019, 9:57 PM

kib added inline comments.Sep 16 2019, 8:29 AM

sys/fs/tmpfs/tmpfs_subr.c
194 ↗	(On Diff #62111)	It would be really nice to save this assert, even in non-precise form.
sys/kern/vfs_mount.c
1410 ↗	(On Diff #62111)	Please add a comment pointing out reciprocal _rel() fence.
1461 ↗	(On Diff #62111)	This sounds as if it is more suitable for DIAGNOSTICS.
sys/kern/vfs_subr.c
721 ↗	(On Diff #62111)	This can only compile under INVARIANTS.
sys/kern/vfs_vnops.c
1807 ↗	(On Diff #62111)	I think it would be better to wrap these zpcpu_get++/-- into macros, where you would also have a place to assert critical section.
1822 ↗	(On Diff #62111)	Again INVARIANTS.

mjg added inline comments.Sep 16 2019, 9:04 AM

sys/fs/tmpfs/tmpfs_subr.c
194 ↗	(On Diff #62111)	The problem is that value of any particular counter is meaningless. It can be arbitrary over the entire range and the sum could still be 0. Going over all CPUs every time would be a little slow and could still give a false-positive of 0, at which point the code would have to disable per-cpu ops to let the counter stabilize. I don't think it's worth it.
sys/kern/vfs_mount.c
1410 ↗	(On Diff #62111)	ok
1461 ↗	(On Diff #62111)	ok
sys/kern/vfs_subr.c
721 ↗	(On Diff #62111)	this compiles just fine because vfs_dump_mount_counters expands to do { } while (0)
sys/kern/vfs_vnops.c
1807 ↗	(On Diff #62111)	This is where my note about helpers comes in. I think the constant need to cast is cumbersome and should be eliminated in a type-checking friendly manner. I was considering adding struct pcpu_int along with a collection of helpers, which would wrap around the zpcpu stuff, but I don't know if that's welcome. For the actual op, there is a problem with consistency. It's hard to come up with good names for these ops when MNT_REF/MNT_REL exists. Perhaps they can all be grouped in something like "vfs_mp_count_add/sub(mp, writeopcount, 1);" and MNT_*_UNLOCKED would get eliminated. This would also assert both critnest and the thread op count present.
1822 ↗	(On Diff #62111)	the printf is only here because the original wanted to report without invariants. I perhaps it's time to drop this part. same with panic elsewhere.

mjg added inline comments.Sep 16 2019, 9:09 AM

sys/kern/vfs_vnops.c
1822 ↗	(On Diff #62111)	The printf is only here because the original code wanted to report the problem even without invariants, same goes for the panic elsewhere. Perhaps it's time to drop this part.

mjg edited the summary of this revision. (Show Details)Sep 16 2019, 9:13 AM

assorted fixes
add vfs_mp_count_add/sub_pcpu in place of hand-rolled manipulation. I don't care for this name, but I think the approach is fine

kib accepted this revision.Sep 16 2019, 6:09 PM

kib added inline comments.

sys/kern/vfs_subr.c
721 ↗	(On Diff #62111)	Where is that define ?
sys/sys/mount.h
1009 ↗	(On Diff #62144)	Is it ever used with val != 1 ? I think you may replace sub with add(-1) instead.

This revision is now accepted and ready to land.Sep 16 2019, 6:09 PM

mjg added inline comments.Sep 16 2019, 6:29 PM

sys/kern/vfs_subr.c
721 ↗	(On Diff #62111)	sys/mount.h:956
sys/sys/mount.h
1009 ↗	(On Diff #62144)	I find having dedicated _sub to cleaner, but in the hader I was considering implementing _sub as add with -val. Ultimately I don't think it's a big deal one way or the other.