The following reply was made to PR kern/175759; it has been noted by GNATS.
From: Andrey Simonenko <si...@comsys.ntu-kpi.kiev.ua>
To: Bruce Evans <b...@optusnet.com.au>
Cc: Gleb Smirnoff <gleb...@freebsd.org>, freebsd-gnats-sub...@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).
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
