On Mon, Jun 10, 2024 at 11:54 AM Steven Rostedt <rost...@goodmis.org> wrote: > > On Thu, 6 Jun 2024 10:37:46 -0500 > Yan Zhai <y...@cloudflare.com> wrote: > > > > name: kfree_skb > > > ID: 1799 > > > format: > > > field:unsigned short common_type; offset:0; size:2; > > > signed:0; > > > field:unsigned char common_flags; offset:2; size:1; > > > signed:0; > > > field:unsigned char common_preempt_count; offset:3; > > > size:1; signed:0; > > > field:int common_pid; offset:4; size:4; signed:1; > > > > > > field:void * skbaddr; offset:8; size:8; signed:0; > > > field:void * location; offset:16; size:8; signed:0; > > > field:unsigned short protocol; offset:24; size:2; signed:0; > > > field:enum skb_drop_reason reason; offset:28; size:4; > > > signed:0; > > > > > > Notice that "protocol" is 2 bytes in size at offset 24, but "reason" > > > starts > > > at offset 28. This means at offset 26, there's a 2 byte hole. > > > > > The reason I added the pointer as the last argument is trying to > > minimize the surprise to existing TP users, because for common ABIs > > it's fine to omit later arguments when defining a function, but it > > needs change and recompilation if the order of arguments changed. > > Nothing should be hard coding the offsets of the fields. This is > exported to user space so that tools can see where the fields are. > That's the purpose of libtraceevent. The fields should be movable and > not affect anything. There should be no need to recompile. > Oh I misunderstood previously. I was also thinking about the argument order in TP_PROTO, but what you mentioned is just the order in TP_STRUCT__entry, correct? I'd prefer to not change the argument order but the struct field order can definitely be aligned better here.
Yan > > > > Looking at the actual format after the change, it does not add a new > > hole since protocol and reason are already packed into the same 8-byte > > block, so rx_skaddr starts at 8-byte aligned offset: > > > > # cat /sys/kernel/debug/tracing/events/skb/kfree_skb/format > > name: kfree_skb > > ID: 2260 > > format: > > field:unsigned short common_type; offset:0; > > size:2; signed:0; > > field:unsigned char common_flags; offset:2; > > size:1; signed:0; > > field:unsigned char common_preempt_count; offset:3; > > size:1; signed:0; > > field:int common_pid; offset:4; size:4; signed:1; > > > > field:void * skbaddr; offset:8; size:8; signed:0; > > field:void * location; offset:16; size:8; signed:0; > > field:unsigned short protocol; offset:24; size:2; signed:0; > > field:enum skb_drop_reason reason; offset:28; > > size:4; signed:0; > > field:void * rx_skaddr; offset:32; size:8; signed:0; > > > > Do you think we still need to change the order? > > Up to you, just wanted to point it out. > > -- Steve >
