From owner-freebsd-arch@FreeBSD.ORG Tue Jul 9 11:06:36 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 18B92F9F for ; Tue, 9 Jul 2013 11:06:36 +0000 (UTC) (envelope-from benno@FreeBSD.org) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mx1.freebsd.org (Postfix) with ESMTP id C934C192A for ; Tue, 9 Jul 2013 11:06:35 +0000 (UTC) Received: from compute1.internal (compute1.nyi.mail.srv.osa [10.202.2.41]) by gateway1.nyi.mail.srv.osa (Postfix) with ESMTP id E381120EDD; Tue, 9 Jul 2013 07:06:34 -0400 (EDT) Received: from frontend2.nyi.mail.srv.osa ([10.202.2.161]) by compute1.internal (MEProxy); Tue, 09 Jul 2013 07:06:34 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=content-type:mime-version:subject:from :in-reply-to:date:cc:content-transfer-encoding:message-id :references:to; s=smtpout; bh=UtMUvExu8X87Im3xZjVLtC8ZLBI=; b=Ss sRqWaCmsFOfaTO4+BzCeKksVU5fYveZN9e0j2WbJitwlgS2WhZX8ma3diddHIKvD grObqUBN7zgqYRv3+1KvE/S5pW5tKN+xk+S+Rg0b+YhEUhXDqz05uJ+X0z7CNZjc ut/ZFpjRLSvO9qAJ3XsQHFav4NWuWdmMp5YgKiv0A= X-Sasl-enc: e7I9qYMu0sfOxeX+nMmtUAEV5OaOHL7KCHtYqZRbJ7Ql 1373367994 Received: from mittlerweile.fritz.box (unknown [118.209.106.43]) by mail.messagingengine.com (Postfix) with ESMTPA id CF665680279; Tue, 9 Jul 2013 07:06:33 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 6.5 \(1508\)) Subject: Re: Reworking vmmeter From: Benno Rice In-Reply-To: <20130707052553.GC91021@kib.kiev.ua> Date: Tue, 9 Jul 2013 21:06:33 +1000 Content-Transfer-Encoding: quoted-printable Message-Id: <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.org> References: <20130707052553.GC91021@kib.kiev.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.1508) Cc: freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jul 2013 11:06:36 -0000 Apologies for the delay, I've been at a conference. Replies inline. On 07/07/2013, at 3:25 PM, Konstantin Belousov = 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?