svn commit: r323393 - in head/sys: sys vm

[Bruce Evans]

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 <sys/counter.h>
>
> +#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 <sys/counter.h>
- 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