On 9/23/20 10:07 PM, Steven Rostedt wrote:
On Wed, 23 Sep 2020 17:15:06 +0200
Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
Add trace points to the Surface Aggregator subsystem core. These trace
points can be used to track packets, requests, and allocations. They are
further intended for debugging and testing/validation, specifically in
combination with the error injection capabilities introduced in the
subsequent commit.
I'm impressed! This uses some of the advanced features of the tracing
infrastructure. But I still have some comments to make about the layout
of the TP_STRUCT__entry() fields.
Thanks!
[...]
+DECLARE_EVENT_CLASS(ssam_packet_class,
+ TP_PROTO(const struct ssh_packet *packet),
+
+ TP_ARGS(packet),
+
+ TP_STRUCT__entry(
+ __array(char, uid, SSAM_PTR_UID_LEN)
+ __field(u8, priority)
+ __field(u16, length)
+ __field(unsigned long, state)
+ __field(u16, seq)
Order matters above to keep the events as compact as possible. The more
compact they are, the more events you can store without loss.
Now with SSAM_PTR_UID_LEN = 9, the above is (on a 64 bit system);
9 bytes;
1 byte;
2 bytes;
8 bytes;
2 bytes;
The ftrace ring buffer is 4 byte aligned. As words and long words are
also 4 byte aligned, there's not much different to change. But it is
possible that the compiler might add 4 byte padding between the long
word "length" and "priority". Note, these are not packed structures.
Testing this out with the following code:
$ cat << EOF > test.c
struct test {
unsigned char array[9];
unsigned char priority;
unsigned short length;
unsigned long state;
unsigned short seq;
};
static struct test x;
void receive_x(struct test *p)
{
p = &x;
}
EOF
$ gcc -g -c -o test.o test.c
$ pahole test.o
struct test {
unsigned char array[9]; /* 0 9 */
unsigned char priority; /* 9 1 */
short unsigned int length; /* 10 2 */
/* XXX 4 bytes hole, try to pack */
long unsigned int state; /* 16 8 */
short unsigned int seq; /* 24 2 */
/* size: 32, cachelines: 1, members: 5 */
/* sum members: 22, holes: 1, sum holes: 4 */
/* padding: 6 */
/* last cacheline: 32 bytes */
};
You do see a hole between length and state. Now if we were to move this
around a little.
$ cat <<EOF > test2.c
struct test {
unsigned long state;
unsigned char array[9];
unsigned char priority;
unsigned short length;
unsigned short seq;
};
static struct test x;
void receive_x(struct test *p)
{
p = &x;
}
EOF
$ gcc -g -c -o test2 test2.c
$ pahole test2.o
struct test {
long unsigned int state; /* 0 8 */
unsigned char array[9]; /* 8 9 */
unsigned char priority; /* 17 1 */
short unsigned int length; /* 18 2 */
short unsigned int seq; /* 20 2 */
/* size: 24, cachelines: 1, members: 5 */
/* padding: 2 */
/* last cacheline: 24 bytes */
};
We get a more compact structure with:
TP_STRUCT__entry(
__field(unsigned long, state)
__array(char, uid, SSAM_PTR_UID_LEN)
__field(u8, priority)
__field(u16, length)
__field(u16, seq)
),
Note, you can find pahole here:
https://git.kernel.org/pub/scm/devel/pahole/pahole.git
+ ),
Thank you for that detailed write-up! As you have clearly noticed, I
have not really looked at the struct layouts. I will fix this for v2,
include your changes, and have a look at pahole.
[...]
Thanks,
Max
