Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH 4/9] surface_aggregator: Add trace points
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh@kernel.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        =?UTF-8?Q?Bla=c5=be_Hrastnik?= <blaz@mxxn.io>,
        Dorian Stoll <dorian.stoll@tmsp.io>
References: <20200923151511.3842150-1-luzmaximilian@gmail.com>
 <20200923151511.3842150-5-luzmaximilian@gmail.com>
 <20200923160757.51e773a0@oasis.local.home>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <dad57e7f-7f3c-7fea-0fbf-a32006da52b6@gmail.com>
Date:   Thu, 24 Sep 2020 01:43:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200923160757.51e773a0@oasis.local.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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