| Message ID | 20250409110705.437321-1-oss@malat.biz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libtraceevent plugins: Add plugin_net to handle bswap during print | expand |
On Wed, 9 Apr 2025 13:07:05 +0200 Petr Malat <oss@malat.biz> wrote: > +static unsigned long long > +process_builtin_bswap16(struct trace_seq *s, unsigned long long *args) > +{ > + return __builtin_bswap16((uint16_t)args[0]); > +} > + > +static unsigned long long > +process_builtin_bswap32(struct trace_seq *s, unsigned long long *args) > +{ > + return __builtin_bswap32((uint32_t)args[0]); > +} > + > +static unsigned long long > +process_builtin_bswap64(struct trace_seq *s, unsigned long long *args) > +{ > + return __builtin_bswap64(args[0]); > +} > + These actually look useful to be built into the main parser itself and not necessarily in a plugin. But I'm not sure we can just translate it directly here. What if the trace.dat is created on a big endian machine, but read on a little endian machine? I don't think we want to do the byte swapping, do we? I think this needs to check the file vs host endianess. If they don't match, I think we don't want to do the swap! if (tep->file_bigendian == tep->host_bigendian) return __builtin_bswap*(args[0]); else return args[0]; ? As the else block will have args[0] already swapped wrt to host that's reading the information. libtraceevent does a byte swap on values read when the host and file do not match endianess. -- Steve
On Wed, 9 Apr 2025 22:56:08 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > As the else block will have args[0] already swapped wrt to host that's > reading the information. libtraceevent does a byte swap on values read when > the host and file do not match endianess. Does this patch work for you? I only compiled it, I haven't tested it against real functions. -- Steve diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h index d3ce0a4..99a7214 100644 --- a/include/traceevent/event-parse.h +++ b/include/traceevent/event-parse.h @@ -181,7 +181,9 @@ struct tep_print_arg_dynarray { struct tep_print_arg *index; }; -struct tep_print_arg; +struct tep_print_arg_bswap { + struct tep_print_arg *field; +}; struct tep_print_arg_op { char *op; @@ -215,6 +217,9 @@ enum tep_print_arg_type { TEP_PRINT_DYNAMIC_ARRAY_LEN, TEP_PRINT_HEX_STR, TEP_PRINT_CPUMASK, + TEP_PRINT_BSWAP16, + TEP_PRINT_BSWAP32, + TEP_PRINT_BSWAP64, }; struct tep_print_arg { @@ -233,6 +238,7 @@ struct tep_print_arg { struct tep_print_arg_bitmask bitmask; struct tep_print_arg_op op; struct tep_print_arg_dynarray dynarray; + struct tep_print_arg_bswap bswap; }; }; diff --git a/src/event-parse.c b/src/event-parse.c index 6317ff6..ae19dee 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -1129,6 +1129,11 @@ static void free_arg(struct tep_print_arg *arg) free_arg(arg->op.left); free_arg(arg->op.right); break; + case TEP_PRINT_BSWAP16: + case TEP_PRINT_BSWAP32: + case TEP_PRINT_BSWAP64: + free_arg(arg->bswap.field); + break; case TEP_PRINT_FUNC: while (arg->func.args) { farg = arg->func.args; @@ -2879,6 +2884,25 @@ static int arg_num_eval(struct tep_print_arg *arg, long long *val) } break; + case TEP_PRINT_BSWAP16: + ret = arg_num_eval(arg->bswap.field, val); + if (!ret) + break; + *val = __builtin_bswap16((uint16_t)(*val)); + return ret; + case TEP_PRINT_BSWAP32: + ret = arg_num_eval(arg->bswap.field, val); + if (!ret) + break; + *val = __builtin_bswap32((uint32_t)(*val)); + return ret; + case TEP_PRINT_BSWAP64: + ret = arg_num_eval(arg->bswap.field, val); + if (!ret) + break; + *val = __builtin_bswap16(*val); + return ret; + case TEP_PRINT_NULL: case TEP_PRINT_FIELD ... TEP_PRINT_SYMBOL: case TEP_PRINT_STRING: @@ -3525,6 +3549,55 @@ out_free: return TEP_EVENT_ERROR; } +static enum tep_event_type +process_builtin_bswap(struct tep_event *event, char *num, struct tep_print_arg *arg, char **tok) +{ + struct tep_handle *tep = event->tep; + struct tep_print_arg *field; + enum tep_event_type type; + char *token = NULL; + int n; + + field = alloc_arg(); + if (!field) { + do_warning_event(event, "%s(%d): not enough memory!", + __func__, __LINE__); + return TEP_EVENT_ERROR; + } + + type = process_arg(event, field, &token); + if (test_type_token(type, token, TEP_EVENT_DELIM, ")")) + goto out_free; + + tep_free_token(token); + token = NULL; + + /* Do not need to swap if host and file do not match */ + if (tep->host_bigendian != tep->file_bigendian) { + *arg = *field; + free_arg(field); + return read_token_item(event->tep, tok); + } + + n = atoi(num); + + switch (n) { + case 16: arg->type = TEP_PRINT_BSWAP16; break; + case 32: arg->type = TEP_PRINT_BSWAP32; break; + case 64: arg->type = TEP_PRINT_BSWAP64; break; + default: goto out_free; + } + + arg->bswap.field = field; + return read_token_item(event->tep, tok); + +out_free: + free_arg(field); + tep_free_token(token); + *tok = NULL; + return TEP_EVENT_ERROR; +} + static enum tep_event_type process_sizeof(struct tep_event *event, struct tep_print_arg *arg, char **tok) { @@ -3696,6 +3769,13 @@ process_function(struct tep_event *event, struct tep_print_arg *arg, tep_free_token(token); return process_sizeof(event, arg, tok); } + if (strncmp(token, "__builtin_bswap", 15) == 0) { + enum tep_event_type type; + + type = process_builtin_bswap(event, token + 15, arg, tok); + tep_free_token(token); + return type; + } func = find_func_handler(event->tep, token); if (func) {
Hi, On Mon, Apr 14, 2025 at 11:41:17AM -0400, Steven Rostedt wrote: > On Wed, 9 Apr 2025 22:56:08 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > As the else block will have args[0] already swapped wrt to host that's > > reading the information. libtraceevent does a byte swap on values read when > > the host and file do not match endianess. > > Does this patch work for you? > > I only compiled it, I haven't tested it against real functions. Sorry for the late reply, I tested my patch on mips, arm, arm64 and x86 and it works everywhere, but for arm64 I had to add extra functions, because it doesn't use the GCC __builin for swapping bytes. I have extended the plugin and will send a new version. On mips using network order ntohs (and similar) expands to a cast only, so the call to swap is not present. BR, Petr
On Thu, 17 Apr 2025 05:06:44 -0400 Petr Malat <oss@malat.biz> wrote: > Sorry for the late reply, I tested my patch on mips, arm, arm64 and x86 and > it works everywhere, but for arm64 I had to add extra functions, because > it doesn't use the GCC __builin for swapping bytes. I have extended the > plugin and will send a new version. > > On mips using network order ntohs (and similar) expands to a cast only, so > the call to swap is not present. So if you record on x86 and read it on mips (or other big endian machines) does it produce the correct output? What about recording on mips and then reading it on x86? That is, do a "trace-cmd record" on a bigendian or little endian machine, then copying the trace.dat file to a machine with the opposite endian, and seeing if it still produces the correct output? BTW, libtraceevent also provides its own swap functions so if the GCC builtin is not always available, then it should use that instead. See the "tep_data2host*()" fuctions. But that may need to be updated as "tep_swapbytes*()" and have: if (!tep || tep->host_bigendian != tep->file_bigendian) return data; Instead. -- Steve
On Thu, 17 Apr 2025 07:49:00 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > BTW, libtraceevent also provides its own swap functions so if the GCC > builtin is not always available, then it should use that instead. > > See the "tep_data2host*()" fuctions. > > But that may need to be updated as "tep_swapbytes*()" and have: > > if (!tep || tep->host_bigendian != tep->file_bigendian) > return data; Something like this: diff --git a/src/event-parse-api.c b/src/event-parse-api.c index 8a0e976ade45..1897117142c9 100644 --- a/src/event-parse-api.c +++ b/src/event-parse-api.c @@ -106,52 +106,80 @@ bool tep_test_flag(struct tep_handle *tep, enum tep_flag flag) return false; } -__hidden unsigned short tep_data2host2(struct tep_handle *tep, unsigned short data) +static inline unsigned short swap2(unsigned short data) { - unsigned short swap; + return ((data & 0xffULL) << 8) | + ((data & (0xffULL << 8)) >> 8); +} +static inline unsigned int swap4(unsigned int data) +{ + return ((data & 0xffULL) << 24) | + ((data & (0xffULL << 8)) << 8) | + ((data & (0xffULL << 16)) >> 8) | + ((data & (0xffULL << 24)) >> 24); +} + +static inline unsigned long long swap8(unsigned long long data) +{ + return ((data & 0xffULL) << 56) | + ((data & (0xffULL << 8)) << 40) | + ((data & (0xffULL << 16)) << 24) | + ((data & (0xffULL << 24)) << 8) | + ((data & (0xffULL << 32)) >> 8) | + ((data & (0xffULL << 40)) >> 24) | + ((data & (0xffULL << 48)) >> 40) | + ((data & (0xffULL << 56)) >> 56); +} + +__hidden unsigned short tep_data2host2(struct tep_handle *tep, unsigned short data) +{ if (!tep || tep->host_bigendian == tep->file_bigendian) return data; - swap = ((data & 0xffULL) << 8) | - ((data & (0xffULL << 8)) >> 8); - - return swap; + return swap2(data); } __hidden unsigned int tep_data2host4(struct tep_handle *tep, unsigned int data) { - unsigned int swap; - if (!tep || tep->host_bigendian == tep->file_bigendian) return data; - swap = ((data & 0xffULL) << 24) | - ((data & (0xffULL << 8)) << 8) | - ((data & (0xffULL << 16)) >> 8) | - ((data & (0xffULL << 24)) >> 24); - - return swap; + return swap4(data); } __hidden unsigned long long tep_data2host8(struct tep_handle *tep, unsigned long long data) { - unsigned long long swap; - if (!tep || tep->host_bigendian == tep->file_bigendian) return data; - swap = ((data & 0xffULL) << 56) | - ((data & (0xffULL << 8)) << 40) | - ((data & (0xffULL << 16)) << 24) | - ((data & (0xffULL << 24)) << 8) | - ((data & (0xffULL << 32)) >> 8) | - ((data & (0xffULL << 40)) >> 24) | - ((data & (0xffULL << 48)) >> 40) | - ((data & (0xffULL << 56)) >> 56); + return swap8(data); +} + +__hidden unsigned short tep_swapbytes2(struct tep_handle *tep, unsigned short data) +{ + if (tep && tep->host_bigendian != tep->file_bigendian) + return data; + + return swap2(data); +} + +__hidden unsigned int tep_swapbytes4(struct tep_handle *tep, unsigned int data) +{ + if (tep && tep->host_bigendian != tep->file_bigendian) + return data; + + return swap4(data); +} + +__hidden unsigned long long +tep_swapbytes8(struct tep_handle *tep, unsigned long long data) +{ + if (tep && tep->host_bigendian != tep->file_bigendian) + return data; - return swap; + return swap8(data); } /** diff --git a/src/event-parse-local.h b/src/event-parse-local.h index d9e9faf649d1..cb2154547069 100644 --- a/src/event-parse-local.h +++ b/src/event-parse-local.h @@ -116,6 +116,10 @@ unsigned short tep_data2host2(struct tep_handle *tep, unsigned short data); unsigned int tep_data2host4(struct tep_handle *tep, unsigned int data); unsigned long long tep_data2host8(struct tep_handle *tep, unsigned long long data); +unsigned short tep_swapbytes2(struct tep_handle *tep, unsigned short data); +unsigned int tep_swapbytes4(struct tep_handle *tep, unsigned int data); +unsigned long long tep_swapbytes8(struct tep_handle *tep, unsigned long long data); + /* access to the internal parser */ int tep_peek_char(struct tep_handle *tep); void tep_init_input_buf(struct tep_handle *tep, const char *buf, unsigned long long size); -- Steve
On Thu, Apr 17, 2025 at 07:49:00AM -0400, Steven Rostedt wrote: > On Thu, 17 Apr 2025 05:06:44 -0400 > Petr Malat <oss@malat.biz> wrote: > > > Sorry for the late reply, I tested my patch on mips, arm, arm64 and x86 and > > it works everywhere, but for arm64 I had to add extra functions, because > > it doesn't use the GCC __builin for swapping bytes. I have extended the > > plugin and will send a new version. > > > > On mips using network order ntohs (and similar) expands to a cast only, so > > the call to swap is not present. > > So if you record on x86 and read it on mips (or other big endian machines) > does it produce the correct output? What about recording on mips and then > reading it on x86? Yes, it works (mips is BE): - when trace is taken on mips, then ntohs expands to a cast and there is no __builin_bswap in the print format. When that trace is read, it's then handled as any other number. So on mips nothing is done and on x86 bytes are swapped to translate the number to native ordering. - when trace is taken on x86, then ntohs expands to byte swapping. When that trace is read on x86 it's swapped and correctly printed. When it's read on mips it's swapped twice - once when read due to different endianess and then again due to bswap in print format and again the correct value is printed. > That is, do a "trace-cmd record" on a bigendian or little endian machine, > then copying the trace.dat file to a machine with the opposite endian, and > seeing if it still produces the correct output? > > BTW, libtraceevent also provides its own swap functions so if the GCC > builtin is not always available, then it should use that instead. > > See the "tep_data2host*()" fuctions. > > But that may need to be updated as "tep_swapbytes*()" and have: > > if (!tep || tep->host_bigendian != tep->file_bigendian) > return data; > For some reason I was not able to get your patch working, it always prints zeroes instead actual value. I tried to fix it, but no luck, only found copy and paste error where you swap 64-bit int as 16-bit. If you want I can share some example trace.dat files. I will send my updated patch as a separate mail. BR, Petr
On Thu, 17 Apr 2025 10:39:12 -0400 Petr Malat <oss@malat.biz> wrote: > Yes, it works (mips is BE): > - when trace is taken on mips, then ntohs expands to a cast and there is no > __builin_bswap in the print format. When that trace is read, it's then > handled as any other number. So on mips nothing is done and on x86 bytes > are swapped to translate the number to native ordering. > - when trace is taken on x86, then ntohs expands to byte swapping. When that > trace is read on x86 it's swapped and correctly printed. When it's read > on mips it's swapped twice - once when read due to different endianess and > then again due to bswap in print format and again the correct value > is printed. Ah, I guess just swapping would always work. * If recorded on x86 and we have __builtin_bswap*() so that the number read into the buffer is not byte swapped, but should be when printed: If read: 0xaabbccdd, it would print: 0xddccbbaa But in the file, it is still how it was read (0xaabbccdd). Now if the trace.dat file is read on MIPS (BE), it would do the conversion when it reads the number: Reads 0xaabbccdd but then converts it to 0xddccbbaa, but since it's BE, that would be read as a number as: 0xaabbccdd. Then the print fmt would have __builtin_bswap32(), and then mips would convert the 0xddccbbaa into 0xaabbccdd, which being BE it would print: 0xddccbbaa By always swapping if the print_fmt says to swap, reading the x86 trace.dat on both x86 and BE MIPS, would produce the same: 0xddccbbaa. Then we would always need to swap. OK, forget my other suggestions, I think this should be fine. > > > > That is, do a "trace-cmd record" on a bigendian or little endian machine, > > then copying the trace.dat file to a machine with the opposite endian, and > > seeing if it still produces the correct output? > > > > BTW, libtraceevent also provides its own swap functions so if the GCC > > builtin is not always available, then it should use that instead. > > > > See the "tep_data2host*()" fuctions. > > > > But that may need to be updated as "tep_swapbytes*()" and have: > > > > if (!tep || tep->host_bigendian != tep->file_bigendian) > > return data; > > > For some reason I was not able to get your patch working, it always prints > zeroes instead actual value. I tried to fix it, but no luck, only found copy > and paste error where you swap 64-bit int as 16-bit. If you want I can share > some example trace.dat files. Hmm, I didn't test it myself (just compiled it). -- Steve
diff --git a/plugins/Makefile b/plugins/Makefile index 4c8cb17..c07315e 100644 --- a/plugins/Makefile +++ b/plugins/Makefile @@ -91,6 +91,7 @@ PLUGINS += plugin_hrtimer.so PLUGINS += plugin_kmem.so PLUGINS += plugin_kvm.so PLUGINS += plugin_mac80211.so +PLUGINS += plugin_net.so PLUGINS += plugin_sched_switch.so PLUGINS += plugin_function.so PLUGINS += plugin_futex.so diff --git a/plugins/plugin_net.c b/plugins/plugin_net.c new file mode 100644 index 0000000..c5eddb5 --- /dev/null +++ b/plugins/plugin_net.c @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: LGPL-2.1 +#include <stdint.h> + +#include "event-parse.h" +#include "trace-seq.h" + +static unsigned long long +process_builtin_bswap16(struct trace_seq *s, unsigned long long *args) +{ + return __builtin_bswap16((uint16_t)args[0]); +} + +static unsigned long long +process_builtin_bswap32(struct trace_seq *s, unsigned long long *args) +{ + return __builtin_bswap32((uint32_t)args[0]); +} + +static unsigned long long +process_builtin_bswap64(struct trace_seq *s, unsigned long long *args) +{ + return __builtin_bswap64(args[0]); +} + +int TEP_PLUGIN_LOADER(struct tep_handle *tep) +{ + tep_register_print_function(tep, + process_builtin_bswap16, + TEP_FUNC_ARG_INT, + "__builtin_bswap16", + TEP_FUNC_ARG_INT, + TEP_FUNC_ARG_VOID); + tep_register_print_function(tep, + process_builtin_bswap32, + TEP_FUNC_ARG_INT, + "__builtin_bswap32", + TEP_FUNC_ARG_INT, + TEP_FUNC_ARG_VOID); + tep_register_print_function(tep, + process_builtin_bswap64, + TEP_FUNC_ARG_LONG, + "__builtin_bswap64", + TEP_FUNC_ARG_LONG, + TEP_FUNC_ARG_VOID); + return 0; +} + +void TEP_PLUGIN_UNLOADER(struct tep_handle *tep) +{ + tep_unregister_print_function(tep, process_builtin_bswap16, + "__builtin_bswap16"); + tep_unregister_print_function(tep, process_builtin_bswap32, + "__builtin_bswap32"); + tep_unregister_print_function(tep, process_builtin_bswap64, + "__builtin_bswap64"); +}
Traced data my be stored in the network order and transformed to the host order at print time by ntohs() or similar functions. These functions end up being implemented by one of __builtin_bswapXX() functions. Support them in the print format string. Signed-off-by: Petr Malat <oss@malat.biz> --- plugins/Makefile | 1 + plugins/plugin_net.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 plugins/plugin_net.c