Date: Sun, 7 Jul 2013 18:16:32 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Benno Rice <benno@FreeBSD.org>, freebsd-arch@FreeBSD.org Subject: Re: Reworking vmmeter Message-ID: <20130707173525.K4305@besplex.bde.org> In-Reply-To: <20130707052553.GC91021@kib.kiev.ua> References: <A4007ED2-FFD8-4003-8196-4E986CD96EC5@FreeBSD.org> <20130707052553.GC91021@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 7 Jul 2013, Konstantin Belousov wrote: > On Sun, Jul 07, 2013 at 09:53:37AM +1000, Benno Rice wrote: >> So I've put together this patch: >> >> http://people.freebsd.org/~benno/vmmeter.diff >> >> This patch does a few things: >> >> - Renames the singleton "cnt" to "vmmeter". >> - Replaces all the per-cpu counters with counter_u64_t. >> - Removes the vmmeter instance from struct pcpu, due to the above mentioned change. >> - Adds includes for vmmeter.h to a few files that were only getting it via pollution in pcpu.h >> - Removes some entries from assym that weren't being used. >> >> This has been tested on amd64 and nothing else right now, I'm more posting this to get general comments on whether people think this is a good idea. My motivation for this was twofold, firstly to rename cnt and secondly to move the counters to the common counter framework. More testing will be done prior to commit. > > Why did you removed the CTASSERT from sys/pcpu.h ? > > Your edits for the inlines in the sys/vmmeter.h are notoriously > violating style. It has some style bugs (mainly verbose names and long lines), but it is the old inlines in sys/vmmeter.h that have gross style bugs. About half of the changes are for things in struct vmmeter that aren't per-CPU counters. Too many atomic ops are needed to access these. These could be changed separately -- keep them in a struct named "cnt" until later. vm/vm_meter.c is uglier than before. The vcnt() hack was at least short. Now with sysctl support for combining the per-CPU parts, it should take less code than before if the counter API and its use were anyy good. Instead it takes 10 times as much to allocate and use all the counters separately. Older design errors for vmmeter: - there should have been a vmmeter sysctl that returns struct vmmeter - instead, there was a VM_METER sysctl that returned struct vmtotal. This andd the VM_LOADAVG were all that 4.4BSD had - FreeBSD added many little sysctls to return the things in struct vmmeter separately. This is very large and slow for both the kernel and applications. - this was unimproved further by renaming VM_METER to VM_TOTAL without preparing to use VM_METER to actually return struct vmmeter. It might have been possible to expand the VM_METER sysctl to return everything. If you change all the little sysctls to return 64-bit counters, then you will have similar API and ABI incompatibilites unless you add compatibility cruft for everything. Size changes are only slightly simpler to handle for scalar sysctls. > You probably could add some macro like COUNTER_U64_INITIALIZED(), which > would check for the counter containing non-NULL pointer. At least this > would allow to remove vmmeter_use_early_counters. Still the hack of > early_*_faults cannot be avoided this way. Or, since BSP is guaranteed > to have id 0, you could temporary put a pointer to the early_*_faults > into the counter_u64_t, which is to be overwritten after the real counter > is initialized. Then the if()s in the vm_fault() can be removed. There are also probems with late faults. v_trap is incremented in trap(), but trap() may be for an NMI interrupting an increment of v_trap. The locking ibased on disabling interrupts for updating counters doesn't work for such counters, unlike PCPU_INC() on 32-bit counters on x86 (PCPU_INC() is broken on most non-x86 arches). > Note that the parts of counters(9) KPI used in your patch has known > issue on some 32 bit arches, namely powerpc32, mips32 and arm. The fetch > could loose the carry bit in a cell, transiently. This is a bug in the > platform implementation, and not the inherent counters(9) limitation. > Fixing requires some asm magic (basically, the counter cells updates and > fetches must be 64bit atomics). This is done on i386 already, and > the problem does not exist on 64bit arches. The bug affects mainly cases that don't work at all with 32-bit counters or with the non-atomic (broken) PCPU_*() accesses on most arches except x86, but I still disagree with using 64-bit counters for the low-level counters. This is pessimal on i386 and causes even larger implemetation problems/pessimizations on non-i386 non-64-bit systems. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130707173525.K4305>