* [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE @ 2025-01-02 17:43 shiju.jose 2025-01-06 17:22 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: shiju.jose @ 2025-01-02 17:43 UTC (permalink / raw) To: rostedt, mhiramat, mathieu.desnoyers, linux-trace-kernel, linux-kernel Cc: linuxarm, jonathan.cameron, tanxiaofei, prime.zeng, shiju.jose From: Shiju Jose <shiju.jose@huawei.com> When userspace reads a trace event format file, the maximum data size that can be read is limited to PAGE_SIZE by the seq_read() and seq_read_iter() functions. This results in userspace receiving partial data if the format file is larger than PAGE_SIZE, requiring a workaround to read the complete data from the format file. Add support for reading trace event format files larger than PAGE_SIZE when needed by userspace. Signed-off-by: Shiju Jose <shiju.jose@huawei.com> --- kernel/trace/trace_events.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index 1545cc8b49d0..ef33614e7f22 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -1801,6 +1801,36 @@ static int trace_format_open(struct inode *inode, struct file *file) return 0; } +/** + * trace_format_read - read() method for format file. + * @file: the file to read from + * @buf: the buffer to read to + * @size: the maximum number of bytes to read + * @ppos: the current position in the file + * + * * Return: + * * %0 - No bytes copied (EOF). + * * %>0 - Number of bytes copied. + * * %<0 - Error code. + */ +static ssize_t trace_format_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) +{ + ssize_t copied = 0; + ssize_t ret; + + do { + ret = seq_read(file, buf + copied, size - copied, ppos); + if (ret < 0) + return ret; + + copied += ret; + if (copied >= size) + break; + } while (ret); + + return copied; +} + #ifdef CONFIG_PERF_EVENTS static ssize_t event_id_read(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos) @@ -2284,7 +2314,7 @@ static const struct file_operations ftrace_enable_fops = { static const struct file_operations ftrace_event_format_fops = { .open = trace_format_open, - .read = seq_read, + .read = trace_format_read, .llseek = seq_lseek, .release = seq_release, }; -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-02 17:43 [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE shiju.jose @ 2025-01-06 17:22 ` Steven Rostedt 2025-01-06 18:15 ` Shiju Jose 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2025-01-06 17:22 UTC (permalink / raw) To: shiju.jose Cc: mhiramat, mathieu.desnoyers, linux-trace-kernel, linux-kernel, linuxarm, jonathan.cameron, tanxiaofei, prime.zeng On Thu, 2 Jan 2025 17:43:17 +0000 <shiju.jose@huawei.com> wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > When userspace reads a trace event format file, the maximum data size > that can be read is limited to PAGE_SIZE by the seq_read() and > seq_read_iter() functions. This results in userspace receiving partial > data if the format file is larger than PAGE_SIZE, requiring a workaround > to read the complete data from the format file. > > Add support for reading trace event format files larger than PAGE_SIZE when > needed by userspace. > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> How is this an issue? This is common for all pseudo files and can be handled properly from user space. Here, with this program: read.c: -------------------------8<------------------------- #include <stdlib.h> #include <stdio.h> #include <unistd.h> #include <fcntl.h> #include <sys/types.h> int main (int argc, char **argv) { char *buf; int fd; off_t size; int r, s; if (argc < 2) { printf("usage: %s file\n", argv[0]); exit(-1); } fd = open(argv[1], O_RDONLY); if (fd < 0) { perror(argv[1]); exit(-1); } size = BUFSIZ * 10; buf = malloc(size); for (s = 0, r = 1; r > 0; s += r) { r = read(fd, buf, size); if (r < 0) { perror(argv[1]); exit(-1); } printf("Read %d bytes from %s\n", r, argv[1]); } free(buf); close(fd); return 0; } ------------------------->8------------------------- $ read /proc/kallsyms Read 4091 bytes from /proc/kallsyms Read 4075 bytes from /proc/kallsyms Read 4078 bytes from /proc/kallsyms Read 4083 bytes from /proc/kallsyms Read 4093 bytes from /proc/kallsyms Read 4076 bytes from /proc/kallsyms Read 4080 bytes from /proc/kallsyms Read 4086 bytes from /proc/kallsyms Read 4080 bytes from /proc/kallsyms Read 4064 bytes from /proc/kallsyms Read 4071 bytes from /proc/kallsyms Read 4063 bytes from /proc/kallsyms Read 4069 bytes from /proc/kallsyms Read 4079 bytes from /proc/kallsyms Read 4063 bytes from /proc/kallsyms Read 4072 bytes from /proc/kallsyms Read 4046 bytes from /proc/kallsyms Read 4091 bytes from /proc/kallsyms Read 4090 bytes from /proc/kallsyms Read 4067 bytes from /proc/kallsyms Read 4080 bytes from /proc/kallsyms Read 4066 bytes from /proc/kallsyms Read 4085 bytes from /proc/kallsyms Read 4095 bytes from /proc/kallsyms Read 4076 bytes from /proc/kallsyms Read 4090 bytes from /proc/kallsyms Read 4066 bytes from /proc/kallsyms Read 4073 bytes from /proc/kallsyms Read 4091 bytes from /proc/kallsyms Read 4075 bytes from /proc/kallsyms Read 4076 bytes from /proc/kallsyms Read 4048 bytes from /proc/kallsyms Read 4074 bytes from /proc/kallsyms Read 4058 bytes from /proc/kallsyms Read 4074 bytes from /proc/kallsyms [..] Read 4052 bytes from /proc/kallsyms Read 4061 bytes from /proc/kallsyms Read 4061 bytes from /proc/kallsyms Read 4053 bytes from /proc/kallsyms Read 4083 bytes from /proc/kallsyms Read 4066 bytes from /proc/kallsyms Read 4093 bytes from /proc/kallsyms Read 4072 bytes from /proc/kallsyms Read 1982 bytes from /proc/kallsyms Read 0 bytes from /proc/kallsyms You see, it requires multiple reads to pull in an entire kernel pseudo file. None of those reads are greater than PAGE_SIZE. Why should trace format files be any different? libtracefs handles this perfectly fine: https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-utils.c#n343 Looks like you are trying to change the kernel to fix a user space bug :-/ NAK! -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-06 17:22 ` Steven Rostedt @ 2025-01-06 18:15 ` Shiju Jose 2025-01-06 22:11 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Shiju Jose @ 2025-01-06 18:15 UTC (permalink / raw) To: Steven Rostedt Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) >-----Original Message----- >From: Steven Rostedt <rostedt@goodmis.org> >Sent: 06 January 2025 17:22 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: mhiramat@kernel.org; mathieu.desnoyers@efficios.com; linux-trace- >kernel@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm ><linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>; >Zengtao (B) <prime.zeng@hisilicon.com> >Subject: Re: [PATCH 1/1] tracing: Support reading trace event format file larger >than PAGE_SIZE > >On Thu, 2 Jan 2025 17:43:17 +0000 ><shiju.jose@huawei.com> wrote: > >> From: Shiju Jose <shiju.jose@huawei.com> >> >> When userspace reads a trace event format file, the maximum data size >> that can be read is limited to PAGE_SIZE by the seq_read() and >> seq_read_iter() functions. This results in userspace receiving partial >> data if the format file is larger than PAGE_SIZE, requiring a >> workaround to read the complete data from the format file. >> >> Add support for reading trace event format files larger than PAGE_SIZE >> when needed by userspace. >> >> Signed-off-by: Shiju Jose <shiju.jose@huawei.com> > >How is this an issue? This is common for all pseudo files and can be handled >properly from user space. > >Here, with this program: > >read.c: >-------------------------8<------------------------- >#include <stdlib.h> >#include <stdio.h> >#include <unistd.h> >#include <fcntl.h> >#include <sys/types.h> > >int main (int argc, char **argv) >{ > char *buf; > int fd; > off_t size; > int r, s; > > if (argc < 2) { > printf("usage: %s file\n", argv[0]); > exit(-1); > } > > fd = open(argv[1], O_RDONLY); > if (fd < 0) { > perror(argv[1]); > exit(-1); > } > > size = BUFSIZ * 10; > > buf = malloc(size); > for (s = 0, r = 1; r > 0; s += r) { > r = read(fd, buf, size); > if (r < 0) { > perror(argv[1]); > exit(-1); > } > printf("Read %d bytes from %s\n", r, argv[1]); > } > free(buf); > close(fd); > return 0; >} >------------------------->8------------------------- > > $ read /proc/kallsyms >Read 4091 bytes from /proc/kallsyms >Read 4075 bytes from /proc/kallsyms >Read 4078 bytes from /proc/kallsyms >Read 4083 bytes from /proc/kallsyms >Read 4093 bytes from /proc/kallsyms >Read 4076 bytes from /proc/kallsyms >Read 4080 bytes from /proc/kallsyms >Read 4086 bytes from /proc/kallsyms >Read 4080 bytes from /proc/kallsyms >Read 4064 bytes from /proc/kallsyms >Read 4071 bytes from /proc/kallsyms >Read 4063 bytes from /proc/kallsyms >Read 4069 bytes from /proc/kallsyms >Read 4079 bytes from /proc/kallsyms >Read 4063 bytes from /proc/kallsyms >Read 4072 bytes from /proc/kallsyms >Read 4046 bytes from /proc/kallsyms >Read 4091 bytes from /proc/kallsyms >Read 4090 bytes from /proc/kallsyms >Read 4067 bytes from /proc/kallsyms >Read 4080 bytes from /proc/kallsyms >Read 4066 bytes from /proc/kallsyms >Read 4085 bytes from /proc/kallsyms >Read 4095 bytes from /proc/kallsyms >Read 4076 bytes from /proc/kallsyms >Read 4090 bytes from /proc/kallsyms >Read 4066 bytes from /proc/kallsyms >Read 4073 bytes from /proc/kallsyms >Read 4091 bytes from /proc/kallsyms >Read 4075 bytes from /proc/kallsyms >Read 4076 bytes from /proc/kallsyms >Read 4048 bytes from /proc/kallsyms >Read 4074 bytes from /proc/kallsyms >Read 4058 bytes from /proc/kallsyms >Read 4074 bytes from /proc/kallsyms >[..] >Read 4052 bytes from /proc/kallsyms >Read 4061 bytes from /proc/kallsyms >Read 4061 bytes from /proc/kallsyms >Read 4053 bytes from /proc/kallsyms >Read 4083 bytes from /proc/kallsyms >Read 4066 bytes from /proc/kallsyms >Read 4093 bytes from /proc/kallsyms >Read 4072 bytes from /proc/kallsyms >Read 1982 bytes from /proc/kallsyms >Read 0 bytes from /proc/kallsyms > >You see, it requires multiple reads to pull in an entire kernel pseudo file. None of >those reads are greater than PAGE_SIZE. Why should trace format files be any >different? Thanks for the reply. Yes. I had a fix/workaround in the userspace rasdaemon with multiple reads like above as reported previously in the following thread. https://lore.kernel.org/lkml/3c9808a694d242cab35bab67602edebf@huawei.com/ However thought a solution in the common kernel code for the format file may be better. I will go ahead with the user space solution. Also shared an information in the above thread about libtraceevent __parse_event() does not return error when parse_format() fail with incomplete formt data, which resulted initialization for the trace event does not fail in the user space tool. >libtracefs handles this perfectly fine: > > https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs- >utils.c#n343 > >Looks like you are trying to change the kernel to fix a user space bug :-/ > > NAK! > >-- Steve Thanks, Shiju ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-06 18:15 ` Shiju Jose @ 2025-01-06 22:11 ` Steven Rostedt 2025-01-07 11:04 ` Shiju Jose 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2025-01-06 22:11 UTC (permalink / raw) To: Shiju Jose Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) On Mon, 6 Jan 2025 18:15:36 +0000 Shiju Jose <shiju.jose@huawei.com> wrote: > >You see, it requires multiple reads to pull in an entire kernel pseudo file. None of > >those reads are greater than PAGE_SIZE. Why should trace format files be any > >different? > Thanks for the reply. > Yes. I had a fix/workaround in the userspace rasdaemon with multiple reads like above > as reported previously in the following thread. > https://lore.kernel.org/lkml/3c9808a694d242cab35bab67602edebf@huawei.com/ > However thought a solution in the common kernel code for the format file > may be better. I will go ahead with the user space solution. That would make it different than every other pseudo file in the kernel. I rather not do that. > > Also shared an information in the above thread about libtraceevent __parse_event() does not > return error when parse_format() fail with incomplete formt data, which resulted initialization for the > trace event does not fail in the user space tool. Hmm, can you send me the the format file that failed? Just the output of "cat /sys/kernel/tracing/events/<system>/<event>/format" will do. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-06 22:11 ` Steven Rostedt @ 2025-01-07 11:04 ` Shiju Jose 2025-01-07 23:02 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Shiju Jose @ 2025-01-07 11:04 UTC (permalink / raw) To: Steven Rostedt Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) [-- Attachment #1: Type: text/plain, Size: 2842 bytes --] >-----Original Message----- >From: Steven Rostedt <rostedt@goodmis.org> >Sent: 06 January 2025 22:12 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: mhiramat@kernel.org; mathieu.desnoyers@efficios.com; linux-trace- >kernel@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm ><linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>; >Zengtao (B) <prime.zeng@hisilicon.com> >Subject: Re: [PATCH 1/1] tracing: Support reading trace event format file larger >than PAGE_SIZE > >On Mon, 6 Jan 2025 18:15:36 +0000 >Shiju Jose <shiju.jose@huawei.com> wrote: > >> >You see, it requires multiple reads to pull in an entire kernel >> >pseudo file. None of those reads are greater than PAGE_SIZE. Why >> >should trace format files be any different? >> Thanks for the reply. >> Yes. I had a fix/workaround in the userspace rasdaemon with multiple >> reads like above as reported previously in the following thread. >> https://lore.kernel.org/lkml/3c9808a694d242cab35bab67602edebf@huawei.c >> om/ However thought a solution in the common kernel code for the >> format file may be better. I will go ahead with the user space >> solution. > >That would make it different than every other pseudo file in the kernel. I rather >not do that. > >> >> Also shared an information in the above thread about libtraceevent >> __parse_event() does not return error when parse_format() fail with >> incomplete formt data, which resulted initialization for the trace event does >not fail in the user space tool. > >Hmm, can you send me the the format file that failed? > >Just the output of "cat /sys/kernel/tracing/events/<system>/<event>/format" >will do. Please find attached format file, which might have failed to parse because of the unsupported formats in the libtraceevent you mentioned in the following thread. https://lore.kernel.org/lkml/20241127104132.6c1729e1@gandalf.local.home/#t Please find the improvement in the libraceevent which I mentioned for not returning error when parsing failed, =========================================== diff --git a/src/event-parse.c b/src/event-parse.c index 0427061..b9264cb 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -7905,9 +7905,14 @@ __parse_event(struct tep_handle *tep, const char *buf, unsigned long size, const char *sys) { - int ret = parse_format(eventp, tep, buf, size, sys); - struct tep_event *event = *eventp; + int ret; + struct tep_event *event; + + ret = parse_format(eventp, tep, buf, size, sys); + if (ret) + return ret; + event = *eventp; if (event == NULL) return ret; ========================================= > >-- Steve Thanks, Shiju [-- Attachment #2: format_cxl_general_media --] [-- Type: application/octet-stream, Size: 5089 bytes --] name: cxl_general_media ID: 1497 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:__data_loc char[] memdev; offset:8; size:4; signed:0; field:__data_loc char[] host; offset:12; size:4; signed:0; field:int log; offset:16; size:4; signed:1; field:uuid_t hdr_uuid; offset:20; size:16; signed:0; field:u64 serial; offset:40; size:8; signed:0; field:u32 hdr_flags; offset:48; size:4; signed:0; field:u16 hdr_handle; offset:52; size:2; signed:0; field:u16 hdr_related_handle; offset:54; size:2; signed:0; field:u64 hdr_timestamp; offset:56; size:8; signed:0; field:u8 hdr_length; offset:64; size:1; signed:0; field:u8 hdr_maint_op_class; offset:65; size:1; signed:0; field:u8 hdr_maint_op_sub_class; offset:66; size:1; signed:0; field:u64 dpa; offset:72; size:8; signed:0; field:u8 descriptor; offset:80; size:1; signed:0; field:u8 type; offset:81; size:1; signed:0; field:u8 sub_type; offset:82; size:1; signed:0; field:u8 transaction_type; offset:83; size:1; signed:0; field:u8 channel; offset:84; size:1; signed:0; field:u32 device; offset:88; size:4; signed:0; field:u8 comp_id[16]; offset:92; size:16; signed:0; field:u64 hpa; offset:112; size:8; signed:0; field:uuid_t region_uuid; offset:120; size:16; signed:0; field:u16 validity_flags; offset:136; size:2; signed:0; field:u8 rank; offset:138; size:1; signed:0; field:u8 dpa_flags; offset:139; size:1; signed:0; field:__data_loc char[] region_name; offset:140; size:4; signed:0; field:u8 cme_threshold_ev_flags; offset:144; size:1; signed:0; field:u32 cme_count; offset:148; size:4; signed:0; print fmt: "memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb len=%d flags='%s' handle=%x related_handle=%x maint_op_class=%u maint_op_sub_class=%u : dpa=%llx dpa_flags='%s' descriptor='%s' type='%s' sub_type='%s' transaction_type='%s' channel=%u rank=%u device=%x validity_flags='%s' comp_id=%s comp_id_pldm_valid_flags='%s' pldm_entity_id=%s pldm_resource_id=%s hpa=%llx region=%s region_uuid=%pUb cme_threshold_ev_flags='%s' cme_count=%x", __get_str(memdev), __get_str(host), REC->serial, __print_symbolic(REC->log, { CXL_EVENT_TYPE_INFO, "Informational" }, { CXL_EVENT_TYPE_WARN, "Warning" }, { CXL_EVENT_TYPE_FAIL, "Failure" }, { CXL_EVENT_TYPE_FATAL, "Fatal" }), REC->hdr_timestamp, &REC->hdr_uuid, REC->hdr_length, __print_flags(REC->hdr_flags, " | ", { ((((1UL))) << (2)), "PERMANENT_CONDITION" }, { ((((1UL))) << (3)), "MAINTENANCE_NEEDED" }, { ((((1UL))) << (4)), "PERFORMANCE_DEGRADED" }, { ((((1UL))) << (5)), "HARDWARE_REPLACEMENT_NEEDED" }, { ((((1UL))) << (6)), "MAINT_OP_SUB_CLASS_VALID" } ), REC->hdr_handle, REC->hdr_related_handle, REC->hdr_maint_op_class, REC->hdr_maint_op_sub_class, REC->dpa, __print_flags(REC->dpa_flags, "|", { ((((1UL))) << (0)), "VOLATILE" }, { ((((1UL))) << (1)), "NOT_REPAIRABLE" } ), __print_flags(REC->descriptor, "|", { ((((1UL))) << (0)), "UNCORRECTABLE_EVENT" }, { ((((1UL))) << (1)), "THRESHOLD_EVENT" }, { ((((1UL))) << (2)), "POISON_LIST_OVERFLOW" } ), __print_symbolic(REC->type, { 0x00, "ECC Error" }, { 0x01, "Invalid Address" }, { 0x02, "Data Path Error" }, { 0x03, "TE State Violation" }, { 0x04, "Scrub Media ECC Error" }, { 0x05, "Adv Prog CME Counter Expiration" }, { 0x06, "CKID Violation" } ), __print_symbolic(REC->sub_type, { 0x00, "Not Reported" }, { 0x01, "Internal Datapath Error" }, { 0x02, "Media Link Command Training Error" }, { 0x03, "Media Link Control Training Error" }, { 0x04, "Media Link Data Training Error" }, { 0x05, "Media Link CRC Error" } ), __print_symbolic(REC->transaction_type, { 0x00, "Unknown" }, { 0x01, "Host Read" }, { 0x02, "Host Write" }, { 0x03, "Host Scan Media" }, { 0x04, "Host Inject Poison" }, { 0x05, "Internal Media Scrub" }, { 0x06, "Internal Media Management" }, { 0x07, "Internal Media Error Check Scrub" }, { 0x08, "Media Initialization" } ), REC->channel, REC->rank, REC->device, __print_flags(REC->validity_flags, "|", { ((((1UL))) << (0)), "CHANNEL" }, { ((((1UL))) << (1)), "RANK" }, { ((((1UL))) << (2)), "DEVICE" }, { ((((1UL))) << (3)), "COMPONENT" }, { ((((1UL))) << (4)), "COMPONENT PLDM FORMAT" } ), __print_hex(REC->comp_id, 0x10), __print_flags(REC->comp_id[0], " | ", { ((((1UL))) << (0)), "PLDM Entity ID" }, { ((((1UL))) << (1)), "Resource ID" } ), (REC->validity_flags & ((((1UL))) << (3)) && REC->validity_flags & ((((1UL))) << (4))) ? (REC->comp_id[0] & ((((1UL))) << (0))) ? __print_hex(&REC->comp_id[1], 6) : "0x00" : "0x00", (REC->validity_flags & ((((1UL))) << (3)) && REC->validity_flags & ((((1UL))) << (4))) ? (REC->comp_id[0] & ((((1UL))) << (1))) ? __print_hex(&REC->comp_id[7], 4) : "0x00" : "0x00", REC->hpa, __get_str(region_name), &REC->region_uuid, __print_flags(REC->cme_threshold_ev_flags, "|", { ((((1UL))) << (0)), "Corrected Memory Errors in Multiple Media Components" }, { ((((1UL))) << (1)), "Exceeded Programmable Threshold" } ), REC->cme_count ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-07 11:04 ` Shiju Jose @ 2025-01-07 23:02 ` Steven Rostedt 2025-01-07 23:19 ` Steven Rostedt 2025-01-08 10:19 ` Shiju Jose 0 siblings, 2 replies; 9+ messages in thread From: Steven Rostedt @ 2025-01-07 23:02 UTC (permalink / raw) To: Shiju Jose Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) On Tue, 7 Jan 2025 11:04:57 +0000 Shiju Jose <shiju.jose@huawei.com> wrote: > Please find attached format file, which might have failed to parse because of the > unsupported formats in the libtraceevent you mentioned in the following thread. > https://lore.kernel.org/lkml/20241127104132.6c1729e1@gandalf.local.home/#t > > Please find the improvement in the libraceevent which I mentioned for not returning error > when parsing failed, > =========================================== > diff --git a/src/event-parse.c b/src/event-parse.c index 0427061..b9264cb 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -7905,9 +7905,14 @@ __parse_event(struct tep_handle *tep, > const char *buf, unsigned long size, > const char *sys) > { > - int ret = parse_format(eventp, tep, buf, size, sys); > - struct tep_event *event = *eventp; > + int ret; > + struct tep_event *event; > + > + ret = parse_format(eventp, tep, buf, size, sys); > + if (ret) > + return ret; > > + event = *eventp; > if (event == NULL) > return ret; Actually, this is the proper patch: diff --git a/src/event-parse.c b/src/event-parse.c index 33ed7fb47fff..f2e50b0e8992 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -7841,7 +7841,7 @@ static enum tep_errno parse_format(struct tep_event **eventp, ret = event_read_format(event); if (ret < 0) { ret = TEP_ERRNO__READ_FORMAT_FAILED; - goto event_parse_failed; + goto event_alloc_failed; } /* As it's OK if it returns that it failed to parse the print format, as it can still read the event. But if it fails to read the fields, then it is basically useless. -- Steve ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-07 23:02 ` Steven Rostedt @ 2025-01-07 23:19 ` Steven Rostedt 2025-01-08 10:34 ` Shiju Jose 2025-01-08 10:19 ` Shiju Jose 1 sibling, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2025-01-07 23:19 UTC (permalink / raw) To: Shiju Jose Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) On Tue, 7 Jan 2025 18:02:24 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > diff --git a/src/event-parse.c b/src/event-parse.c > index 33ed7fb47fff..f2e50b0e8992 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -7841,7 +7841,7 @@ static enum tep_errno parse_format(struct tep_event **eventp, > ret = event_read_format(event); > if (ret < 0) { > ret = TEP_ERRNO__READ_FORMAT_FAILED; > - goto event_parse_failed; > + goto event_alloc_failed; > } > > /* > > > As it's OK if it returns that it failed to parse the print format, as it > can still read the event. But if it fails to read the fields, then it is > basically useless. And the event you posted would still succeed. And it should succeed actually. It just failed to parse the print format. There's a lot of events that do as the print format is pretty much full C code, and libtraceveent is not a full C parser. It can also call functions that it doesn't know about which means even if it were a full C parser it would still fail to parse. But it does flag the event as "TEP_EVENT_FL_FAILED", and should display "[FAILED_TO_PARSE]" when printing the generic output. I still think failing to parse the fields is more of a critical error though. That shouldn't happen, and if it does, it should be corrected. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-07 23:19 ` Steven Rostedt @ 2025-01-08 10:34 ` Shiju Jose 0 siblings, 0 replies; 9+ messages in thread From: Shiju Jose @ 2025-01-08 10:34 UTC (permalink / raw) To: Steven Rostedt Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) >-----Original Message----- >From: Steven Rostedt <rostedt@goodmis.org> >Sent: 07 January 2025 23:19 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: mhiramat@kernel.org; mathieu.desnoyers@efficios.com; linux-trace- >kernel@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm ><linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>; >Zengtao (B) <prime.zeng@hisilicon.com> >Subject: Re: [PATCH 1/1] tracing: Support reading trace event format file larger >than PAGE_SIZE > >On Tue, 7 Jan 2025 18:02:24 -0500 >Steven Rostedt <rostedt@goodmis.org> wrote: > >> diff --git a/src/event-parse.c b/src/event-parse.c index >> 33ed7fb47fff..f2e50b0e8992 100644 >> --- a/src/event-parse.c >> +++ b/src/event-parse.c >> @@ -7841,7 +7841,7 @@ static enum tep_errno parse_format(struct >tep_event **eventp, >> ret = event_read_format(event); >> if (ret < 0) { >> ret = TEP_ERRNO__READ_FORMAT_FAILED; >> - goto event_parse_failed; >> + goto event_alloc_failed; >> } >> >> /* >> >> >> As it's OK if it returns that it failed to parse the print format, as >> it can still read the event. But if it fails to read the fields, then >> it is basically useless. > >And the event you posted would still succeed. And it should succeed actually. It >just failed to parse the print format. There's a lot of events that do as the print >format is pretty much full C code, and libtraceveent is not a full C parser. It can >also call functions that it doesn't know about which means even if it were a full C >parser it would still fail to parse. > >But it does flag the event as "TEP_EVENT_FL_FAILED", and should display >"[FAILED_TO_PARSE]" when printing the generic output. > Ok. >I still think failing to parse the fields is more of a critical error though. That >shouldn't happen, and if it does, it should be corrected. As reported previously, parsing fields succeed in the rasdaemon for CXL events, when format file is larger than PAGE_SIZE, with the rasdaemon change for reading complete format file. > >-- Steve Thanks, Shiju ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE 2025-01-07 23:02 ` Steven Rostedt 2025-01-07 23:19 ` Steven Rostedt @ 2025-01-08 10:19 ` Shiju Jose 1 sibling, 0 replies; 9+ messages in thread From: Shiju Jose @ 2025-01-08 10:19 UTC (permalink / raw) To: Steven Rostedt Cc: mhiramat@kernel.org, mathieu.desnoyers@efficios.com, linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org, Linuxarm, Jonathan Cameron, tanxiaofei, Zengtao (B) >-----Original Message----- >From: Steven Rostedt <rostedt@goodmis.org> >Sent: 07 January 2025 23:02 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: mhiramat@kernel.org; mathieu.desnoyers@efficios.com; linux-trace- >kernel@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm ><linuxarm@huawei.com>; Jonathan Cameron ><jonathan.cameron@huawei.com>; tanxiaofei <tanxiaofei@huawei.com>; >Zengtao (B) <prime.zeng@hisilicon.com> >Subject: Re: [PATCH 1/1] tracing: Support reading trace event format file larger >than PAGE_SIZE > >On Tue, 7 Jan 2025 11:04:57 +0000 >Shiju Jose <shiju.jose@huawei.com> wrote: > >> Please find attached format file, which might have failed to parse >> because of the unsupported formats in the libtraceevent you mentioned in the >following thread. >> >https://lore.kernel.org/lkml/20241127104132.6c1729e1@gandalf.local.home/#t >> >> Please find the improvement in the libraceevent which I mentioned for >> not returning error when parsing failed, >> =========================================== >> diff --git a/src/event-parse.c b/src/event-parse.c index >> 0427061..b9264cb 100644 >> --- a/src/event-parse.c >> +++ b/src/event-parse.c >> @@ -7905,9 +7905,14 @@ __parse_event(struct tep_handle *tep, >> const char *buf, unsigned long size, >> const char *sys) >> { >> - int ret = parse_format(eventp, tep, buf, size, sys); >> - struct tep_event *event = *eventp; >> + int ret; >> + struct tep_event *event; >> + >> + ret = parse_format(eventp, tep, buf, size, sys); >> + if (ret) >> + return ret; >> >> + event = *eventp; >> if (event == NULL) >> return ret; > >Actually, this is the proper patch: > >diff --git a/src/event-parse.c b/src/event-parse.c index >33ed7fb47fff..f2e50b0e8992 100644 >--- a/src/event-parse.c >+++ b/src/event-parse.c >@@ -7841,7 +7841,7 @@ static enum tep_errno parse_format(struct tep_event >**eventp, > ret = event_read_format(event); > if (ret < 0) { > ret = TEP_ERRNO__READ_FORMAT_FAILED; >- goto event_parse_failed; >+ goto event_alloc_failed; > } > > /* > > >As it's OK if it returns that it failed to parse the print format, as it can still read >the event. But if it fails to read the fields, then it is basically useless. Thanks. Your patch worked fine for returning error to the user space tool for the incomplete format data case as the following check become true in the __parse_event() after freeing 'event' in the goto event_alloc_failed. if (event == NULL) return ret; > >-- Steve Thanks, Shiju ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-08 10:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-02 17:43 [PATCH 1/1] tracing: Support reading trace event format file larger than PAGE_SIZE shiju.jose 2025-01-06 17:22 ` Steven Rostedt 2025-01-06 18:15 ` Shiju Jose 2025-01-06 22:11 ` Steven Rostedt 2025-01-07 11:04 ` Shiju Jose 2025-01-07 23:02 ` Steven Rostedt 2025-01-07 23:19 ` Steven Rostedt 2025-01-08 10:34 ` Shiju Jose 2025-01-08 10:19 ` Shiju Jose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).