Date: Tue, 9 Jul 2013 21:06:33 +1000 From: Benno Rice <benno@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-arch@freebsd.org Subject: Re: Reworking vmmeter Message-ID: <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.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
Apologies for the delay, I've been at a conference. Replies inline. On 07/07/2013, at 3:25 PM, Konstantin Belousov <kostikbel@gmail.com> = wrote: > On Sun, Jul 07, 2013 at 09:53:37AM +1000, Benno Rice wrote: >> So I've put together this patch: >>=20 >> http://people.freebsd.org/~benno/vmmeter.diff >>=20 >> This patch does a few things: >>=20 >> - 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. >>=20 >> 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. >>=20 >=20 > Why did you removed the CTASSERT from sys/pcpu.h ? Because it was no longer a multiple of PAGE_SIZE but significantly less. = I'm open to how to approach this but it seemed that since we were now = less than PAGE_SIZE it was less important. > Your edits for the inlines in the sys/vmmeter.h are notoriously > violating style. I converted them from four-space to one-tab indents, I thought that made = them more compliant rather than less. > 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. I went through a few iterations of how to approach these. Your approach = might be slightly neater but pretty much everything feels mildly = hackish. If there was some way we could allow counters to be allocated = in an "early" mode before the pcpu stuff was available and then = "upgraded" later that could be neater but I'm not sure the complexity is = worth it. > 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. Ok, are you looking at fixing that for these architectures or is this = something that's waiting on a solution?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5A9BF932-4B00-4CE6-AC11-1673CD57A8CF>