From owner-svn-src-all@freebsd.org Mon Sep 11 01:56:31 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 00D89E117C4; Mon, 11 Sep 2017 01:56:30 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 275CB6A374; Mon, 11 Sep 2017 01:56:30 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id F048D1A28D8; Mon, 11 Sep 2017 11:56:21 +1000 (AEST) Date: Mon, 11 Sep 2017 11:56:21 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r323393 - in head/sys: sys vm In-Reply-To: <201709101900.v8AJ0c2N059845@repo.freebsd.org> Message-ID: <20170911111127.B870@besplex.bde.org> References: <201709101900.v8AJ0c2N059845@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=pGeDZM4jIChbch_9Z3sA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Sep 2017 01:56:31 -0000 On Sun, 10 Sep 2017, Mateusz Guzik wrote: > Log: > Move vmmeter atomic counters into dedicated cache lines > > Prior to the change they were subject to extreme false sharing. > In particular this change shaves about 3 seconds real time of -j 80 buildkernel. Changes that small are hard to measure. Kernel builds take about 3 seconds real time altogether (cheating slightly -- this is for a FreeBSD-4 kernel where the sources are held constant to provide a benchmark and a bloatometer for newer kernels and userlands which take 10-20 times longer). > Modified: head/sys/sys/vmmeter.h > ============================================================================== > --- head/sys/sys/vmmeter.h Sun Sep 10 18:08:25 2017 (r323392) > +++ head/sys/sys/vmmeter.h Sun Sep 10 19:00:38 2017 (r323393) > @@ -60,6 +60,12 @@ struct vmtotal { > #if defined(_KERNEL) || defined(_WANT_VMMETER) > #include > > +#ifdef _KERNEL > +#define VMMETER_ALIGNED __aligned(CACHE_LINE_SIZE) > +#else > +#define VMMETER_ALIGNED > +#endif > + Using this breaks the "ABI" which is used by at least vmstat(1). This has some style bugs: - verbose macro name. Using this unimproves the formatting later. - space instead of tab after #define. This vfile is vaguely KNF-conformant and had only 1 instance of this style bug before (for the idempotency ifndef, which has 3 other style bugs: - missing blank line before its #endif - tab instead of space before the comment on its #endif - backwards comment on its #endif). > /* > * System wide statistics counters. > * Locking: > @@ -126,14 +132,15 @@ struct vmmeter { > u_int v_free_target; /* (c) pages desired free */ > u_int v_free_min; /* (c) pages desired free */ > u_int v_free_count; /* (f) pages free */ > - u_int v_wire_count; /* (a) pages wired down */ > - u_int v_active_count; /* (q) pages active */ > u_int v_inactive_target; /* (c) pages desired inactive */ > - u_int v_inactive_count; /* (q) pages inactive */ > - u_int v_laundry_count; /* (q) pages eligible for laundering */ Moving these also breaks the "ABI". > u_int v_pageout_free_min; /* (c) min pages reserved for kernel */ > u_int v_interrupt_free_min; /* (c) reserved pages for int code */ > u_int v_free_severe; /* (c) severe page depletion point */ > + u_int v_wire_count VMMETER_ALIGNED; /* (a) pages wired down */ The more complicated declaration unimproves the formatting, especially since the macro name is so long. Is there a technical reason to move to the end? style(9) requires sorting on alignment, with the most aligned fields first, not last (it says to sort on size, but means alignment). Is there a reason to reverse this here. > + u_int v_active_count VMMETER_ALIGNED; /* (a) pages active */ > + u_int v_inactive_count VMMETER_ALIGNED; /* (a) pages inactive */ > + u_int v_laundry_count VMMETER_ALIGNED; /* (a) pages eligible for > + laundering */ This does more than move the other fields and unimprove their formatting. It also changes the lock annotations from (q) to (a). The formatting is ugliest for the last field. Naming the macro something like VMMA would things fit better. It is stupid to abbreviate VIRTUAL_MEMORY to VM but spell out ALIGNED, especially when the former is global and the latter is local. If the namespace were documented, then the documentation would say that vm, v_ and VM* are reserved, perhaps without underscores, so it would not be namespace pollution to use VMMA. The full undocumented namespace in this file seems to be: - MAXSLP - vm* (used without an underscore for struct vmtotal - t_* - unknown nested pollution in - v_* - VM_* But no VM* without an underscore before VMMETER_ALIGNED. I don't know of any man page where this is undocumented. The only man pages that refers to vmmeter.h is vm_set_page_size(9). The only other man page that refers to vm_cnt is memguard(9). Bruce