Reworking vmmeter

[Benno Rice]

Apologies for the delay, I've been at a conference. Replies inline.

On 07/07/2013, at 3:25 PM, Konstantin Belousov <kostikbel at gmail.com> 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 ?

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?