Date: Tue, 9 Jul 2013 15:54:15 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Benno Rice <benno@FreeBSD.org> Cc: freebsd-arch@freebsd.org Subject: Re: Reworking vmmeter Message-ID: <20130709125415.GK91021@kib.kiev.ua> In-Reply-To: <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.org> References: <A4007ED2-FFD8-4003-8196-4E986CD96EC5@FreeBSD.org> <20130707052553.GC91021@kib.kiev.ua> <5A9BF932-4B00-4CE6-AC11-1673CD57A8CF@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <kostikbel@gmail.com> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130709125415.GK91021>