lkml.org 
[lkml]   [2020]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 4/9] surface_aggregator: Add trace points
From
Date
On 9/23/20 10:07 PM, Steven Rostedt wrote:
> On Wed, 23 Sep 2020 17:15:06 +0200
> Maximilian Luz <luzmaximilian@gmail.com> 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

\
 
 \ /
  Last update: 2020-09-24 01:43    [W:0.130 / U:2.812 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site