From owner-freebsd-arch@FreeBSD.ORG Tue Jul 9 12:54:28 2013 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 43E23B47; Tue, 9 Jul 2013 12:54:28 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) by mx1.freebsd.org (Postfix) with ESMTP id AB3EC1E23; Tue, 9 Jul 2013 12:54:27 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.7/8.14.7) with ESMTP id r69CsFVH027132; Tue, 9 Jul 2013 15:54:15 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua r69CsFVH027132 Received: (from kostik@localhost) by tom.home (8.14.7/8.14.7/Submit) id r69CsFwa027131; Tue, 9 Jul 2013 15:54:15 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 9 Jul 2013 15:54:15 +0300 From: Konstantin Belousov To: Benno Rice Subject: Re: Reworking vmmeter Message-ID: <20130709125415.GK91021@kib.kiev.ua> References: <20130707052553.GC91021@kib.kiev.ua> <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OLEmQOz5exAyrbxI" Content-Disposition: inline In-Reply-To: <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on tom.home 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 12:54:28 -0000 --OLEmQOz5exAyrbxI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 09, 2013 at 09:06:33PM +1000, Benno Rice wrote: > Apologies for the delay, I've been at a conference. Replies inline. >=20 > On 07/07/2013, at 3:25 PM, Konstantin Belousov wrot= e: >=20 > > 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 ment= ioned 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 pos= ting this to get general comments on whether people think this is a good id= ea. 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 ? >=20 > 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. >=20 The assert did not verified that size of struct pcpu is multiple of page size. Please re-read the comment above. You must properly pad struct pcpu, using md fields. > > Your edits for the inlines in the sys/vmmeter.h are notoriously > > violating style. >=20 > I converted them from four-space to one-tab indents, I thought that made = them more compliant rather than less. >=20 Style recommends to fill the line instead of placing single '(' on it. The continuation lines should have additional 4-space indent. > > 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 count= er > > is initialized. Then the if()s in the vm_fault() can be removed. >=20 > I went through a few iterations of how to approach these. Your approach m= ight 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" m= ode before the pcpu stuff was available and then "upgraded" later that coul= d be neater but I'm not sure the complexity is worth it. >=20 It still feels hackish, but it avoids adding the if () conditional to the (hot) vm_fault() path. > > 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. >=20 > Ok, are you looking at fixing that for these architectures or is this som= ething that's waiting on a solution? I can learn and write required assembler, but I cannot test due to lack of hardware. My calls for test for previous work usually end with silence, so I decided to not care about other architectures more than corresponding maintainers. --OLEmQOz5exAyrbxI Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (FreeBSD) iQIbBAEBAgAGBQJR3Af2AAoJEJDCuSvBvK1BzWwP+PaYbaFO1BGi4USBztnvnXCS /x15swljrx92MjO97yV3tQNj63z28Dz/bGDx/Pmzl63KeU+Q3PauRnt/6YBxSQUp np7dNSRyXIlkdKxcSe8Pm1USJgJ5em+JXJYbgICPcQFPo61kNzLGnX06faUeRdo9 Rz86X42FQENyEL/w9wHxgl9m/PAfAnbUokhesCXtUYjlIv6QrwXh32quNjiHq0v/ GNvVE6yBP1+8GMl+JdeKscp5iLhY2Ah7mswtbRjbF2P1/COva099jNirJS2k9NKB F3Au10xM1LF0h6mX2naUzbfmGXZ8C5EGigZI35ft6+6FBytawDiiStn0s2zonVhb cK26oUIeoV3nh3o51MBLPh7vw98kK7ADCPkF4nuJ4R78YH0WdAncbfj39oMEbROl AYaOr967kURyU4N1OiwhR4EltoOAy59jCSghlo1UnsOFnTXvgXuDTVxVwhlXBFTx oEvarGHvWl76Ro0atnAH9fy1KgsSJYghRskrspNw+gW+U2PTworGAdRk6jd6s6bL r80+UCRIRvMhwECo9LJCIqwmJp7rqIAQe+TISZq/wogDsL7TChA/RED9w9McpnxP PUpvzXogrvA1mdG3DG4Tn04HR52AnmR07a/j6GlxNTwvKvPlxcNL47NE/n+dej0P m6mpwTeKJhK1sMcnuXE= =YGLu -----END PGP SIGNATURE----- --OLEmQOz5exAyrbxI--