kern/175759: Correct data types for fields of struct qm_trace{} from <sys/queue.h>

[Andrey Simonenko]

The following reply was made to PR kern/175759; it has been noted by GNATS.

From: Andrey Simonenko <simon at comsys.ntu-kpi.kiev.ua>
To: Bruce Evans <brde at optusnet.com.au>
Cc: Gleb Smirnoff <glebius at FreeBSD.org>, FreeBSD-gnats-submit at freebsd.org
Subject: Re: kern/175759: Correct data types for fields of struct qm_trace{}
 from <sys/queue.h>
Date: Tue, 5 Feb 2013 13:35:50 +0200

 Bruce Evans wrote:
 
 > Unsigned long is unnecessarily large.  It wastes space on 64-bit
 > arches.  The change doesn't change the wastage, because space was
 > already wasted on 64-bit arches by mispacking the struct (with
 > unnamed padding after the ints).  It changes the API unnecessarily
 > by changing signed variables to unsigned.  Sign variables are
 > easier to use, and changing to unsigned ones risks sign extension
 > bugs.
 
 I did not change order of fields to not change API, that's why
 bigger data type is used without changing size of that structure
 due to padding (at least for current sizes of int and long).
 
 > According to your quote of the C standard, int32_t is enough.  (I
 > couldn't find anything directly about either the type or limit of
 > __LINE__ in the n869.txt draft of C99, but #line is limited to 2**31-1.
 > n1124.pdf says much the same, except it says that __LINE__ is an integer
 > constant where n869.txt says that __LINE__ is a decimal constant.  Both
 > of these seem to be wrong -- "decimal constants" include floating point
 > ones, and "integer constants" include octal and hex ones.)
 
 Using [u]int32_t is enough. but it will require to include <stdint.h>
 if QUEUE_MACRO_DEBUG is defined.  By the way including just <sys/queue.h>
 is not enough because it does not include <stddef.h> for NULL.
 
 > The change preserves many style bugs:
 > - no prefix in member names
 > - member names not sorted inversely on size.  This gives the space
 >    wastage.
 > - member names not indented
 > - space between '*' and member names.
 
 I'll repeat my idea how to improve QUEUE_MACRO_DEBUG, I did not
 post it in the first message, because it contains more changes:
 
 1. Allow a user to specify how many places to remember via QMD_TRACE_N.
    by default this value is 2 (as now).
 
 2. Initialize struct qm_trace{} in *_INIT() and *_INITIALIZER(),
    all initialized HEADs will have defined values in trace information.
 
 3. All trace information is saved in the info[QMD_TRACE_N] array, each
    element is a structure with file name and line number.  If QMD_TRACE_N
    is small, then this array can be split into two arrays to save space;
    if QMD_TRACE_N is big, then one array will work faster because of CPU
    cache lines.
 
 4. Lists' elements usually are not initialized (via bzero() for example),
    so first entry in the info[] array will have some random index for
    lists' elements.
 
 5. Using info[] array allows to specify arbitrary number of elements and
    does not require to move data from one element into another (as now).
    Position in array is (idx = (idx + 1) % QMD_TRACE_N) and during
    debugging at will be necessary to look at info[idx] to find the most
    recent change of HEAD or element.
 
 So, the main change is (I improved it, because of optimization):
 
 #define	QMD_TRACE(x) do {						\
 	unsigned int __idx;						\
 	__idx = ((x)->trace.idx + 1) % QMD_TRACE_N;			\
 	(x)->trace.idx = __idx;						\
 	(x)->trace.info[__idx].file = __FILE__;				\
 	(x)->trace.info[__idx].line = __LINE__;				\
 } while (0)
 
 It cam seem that this change makes QUEUE_MACRO_DEBUG more complex.
 Actually using info[] array with 2 elements (default value) should work
 faster than current implementation, since it does not require to copy
 data lastline -> prevline and lastfile -> prevfile, and that for(;;) in
 QMD_TRACE_INIT() for 1 element should be unrolled.
 
 Comparing generated code for QMD_TRACE under FreeBSD/amd64 9.1-STABLE
 by gcc from the base system and by clang-3.2 from Ports for the following
 file with -DQMD_TRACE_N=2^x (for non 2^x values the code will be more
 complex) and "-O2 -DQUEUE_MACRO_DEBUG":
 
 ------
 #include <stddef.h>
 #ifdef QUEUE_DEFAULT
 # include <sys/queue.h>
 #else
 # include "queue.h"
 #endif
 
 struct foo {
 	int x;
 	int y;
 	TAILQ_ENTRY(foo) link;
 };
 
 TAILQ_HEAD(foo_tailq, foo);
 
 void
 func(struct foo_tailq *foo_tailq, struct foo *foo)
 {
 	TAILQ_REMOVE(foo_tailq, foo, link);
 }
 ------
 
 1. GCC: 
 
    Now:    6 cmds: 4 mov to mem, 2 mov from mem.
    New: 9-10 cmds: 3 mov to mem, 1 mov from mem, 5 cmds with regs.
 
 2. clang:
 
    Now:    6 cmds: 4 mov to mem, 2 mov from mem.
    New:    7 cmds: 3 mov to mem, 1 mov from mem, 3 cmds with regs.
 
 The diff can be found here:
 
 http://lists.freebsd.org/pipermail/freebsd-bugs/2013-February/051665.html
 
 It contains many lines, because I renamed some debugging related macro
 names to improve style, added debugging to TAILQ_SWAP(), corrected
 TAILQ_INSERT_AFTER() and TAILQ_INSERT_BEFORE() (added parentheses for
 listelm).