Re: [RFC][PATCH 0/5] tracing/events: stable tracepoints

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> We also have name (redundant), ID (should be agnostic), and print_fmt
> (lots of issues).
> 
> So the new format looks like this:
> 
> [root@bxf ~]# cat /sys/kernel/event/sched_switch/format 
> 	array:prev_comm	type:char	size:8	count:16	align:1	signed:1;
> 	field:prev_pid	type:pid_t	size:32	align:4	signed:1;
> 	field:prev_state	type:char	size:8	align:1	signed:1;
> 	array:next_comm	type:char	size:8	count:16	align:1	signed:1;
> 	field:next_pid	type:pid_t	size:32	align:4	signed:1;

Hrm, this is mixing field and type definitions. How about we organize this in
something that will be both parseable and extensible ?

First, I don't see what exporting the kernel-internal type "pid_t" in there
gives you. Userspace knows nothing about this, so it seems pretty useless.

What do you think of this alternative layout ?

Named types below:

% cat /sys/kernel/event/types/char
parent = integer;
size = 8;
signed = true;
align = 8;

% cat /sys/kernel/event/types/pid_t
parent = integer;
size = 32;
signed = true;
align = 32;     /* Or 8 if the architecture supports unaligned writes
                   efficiently */


% cat /sys/kernel/event/sched_switch/format
type {  /* Nameless type */
        parent = struct;
        fields = {
                {
                        type {  /* Nameless type */
                                parent = array;
                                length = 16;
                                elem_type = char; /* refers to named type */
                        },
                        prev_comm,
                },
                { pid_t, prev_pid, },
                { char, prev_state, },
                {
                        type {  /* Nameless type */
                                parent = array;
                                length = 16;
                                elem_type = char; /* refers to named type */
                        },
                        next_comm,
                },
                { pid_t, next_pid, },
        };
}

With this layout, we can declare types like enumerations, e.g.

% cat /sys/kernel/event/types/trap_id_t
type {
        parent = enum;
        size = 5;       /* 5-bit bitfield to hold the enumeration */
        signed = false;
        align = 1;      /* bit-packed */
        map = {
                { 0, "divide error" },
                { 2, "nmi stack" },
                { 4, "overflow" },
                ....
        };
}

So we can refer to this "named type" in all events for which we want to save
trap ID ? We therefore get the mapping to a human-understandable name for free.

> Some notes:
> 
> o  The size is in bits.

Yep, this will immensely help when dealing with bitfields.

> o  We added an align, that is the natural alignment for the arch of that
>    type.

Just watch out, in your initial example, I think your align field is in bytes
rather than bits. Ideally we'd like everything to be consistent.

Thanks,

Mathieu

> o  We added an "array" type, that specifies the size of an element as
>    well as a "count", where total size can be align(size) * count.
> o  We separated the field name from the type.
> 
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com