| Message ID | 20210115130336.2520663-4-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add sysfs interface to collect reports from debugging tools | expand |
On Fri, Jan 15, 2021 at 02:03:34PM +0100, Alexander Potapenko wrote: > Add ABI documentation for files in /sys/kernel/error_report/ > > Requested-by: Andrew Morton <akpm@linux-foundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Andrey Konovalov <andreyknvl@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Marco Elver <elver@google.com> > Cc: Petr Mladek <pmladek@suse.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Cc: linux-mm@kvack.org > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > .../ABI/testing/sysfs-kernel-error_report | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-kernel-error_report > > diff --git a/Documentation/ABI/testing/sysfs-kernel-error_report b/Documentation/ABI/testing/sysfs-kernel-error_report > new file mode 100644 > index 000000000000..666d039f93a9 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-kernel-error_report > @@ -0,0 +1,41 @@ > +What: /sys/kernel/error_report/ > +Date: January 2021 > +Contact: Alexander Potapenko <glider@google.com>, > + Marco Elver <elver@google.com> > +Description: > + /sys/kernel/error_report/ contains two files: "report_count" > + and "last_report". These files are used to notify userspace > + about error reports from the enrolled kernel subsystems (those > + that use error_report_start/error_report_end tracepoints). > + > + "report_count" contains the current number of reported errors. > + This number is incremented every time the error_report_end > + trace event occurs in the kernel. > + > + "last_report" contains the most recent error report; concurrent > + report generation results in collection of any one report > + ("last_report" may not be the last shown on the console). > + A "report" is everything the task had printed to the console > + between issuing the error_report_start and error_report_end > + trace events. > + > + Due to sysfs limitations, the report size is truncated at > + PAGE_SIZE. To save space, the leading info in square brackets > + printed by CONFIG_PRINTK_TIME and CONFIG_PRINTK_CALLER is > + trimmed from the output lines. > + > + Both files use sysfs_notify() to notify userspace about > + changes. Userspace programs can use poll() to block until an > + error is reported: > + > + pfd.fd = fd; > + pfd.events = POLLPRI; > + while (1) { > + lseek(pfd.fd, 0, SEEK_SET); > + poll(&pfd, 1, -1); > + read(pfd.fd, buffer, PAGE_SIZE); > + /* Process the report in @buffer. */ > + } > + > + Files in /sys/kernel/error_report/ are available when > + CONFIG_ERROR_REPORT_NOTIFY is enabled. sysfs is "one value per file", please put something like this in tracefs, as there is no such rules there. Or debugfs, but please, not sysfs. Also, any reason you didn't cc: the sysfs maintainers? thanks, greg k-h
> sysfs is "one value per file" What about existing interfaces that even export binary blobs through sysfs (e.g. /sys/firmware, /sys/boot_params)? What qualifies as a "value" in that case? > please put something like this in > tracefs, as there is no such rules there. Or debugfs, but please, not > sysfs. Does tracefs have anything similar to sysfs_notify() or any other way to implement a poll() handler? Our main goal is to let users wait on poll(), so that they don't have to check the file for new contents every now and then. Is it possible with tracefs or debugfs? Also, for our goal debugfs isn't a particularly good fit, as Android kernels do not enable debugfs. Not sure about tracefs, they seem to have it, need to check. Do you think it is viable to keep /sys/kernel/error_report/report_count in sysfs and use it for notifications, but move last_report somewhere else? (I'd probably prefer procfs, but it could be tracefs as well, if you find that better). This way it will still be possible to easily notify userspace about new reports, but the report itself won't have any atomicity guarantees. Users will have to check the report count to ensure it didn't change under their feet. > Also, any reason you didn't cc: the sysfs maintainers? Only my lack of common sense :) I'll add them should the following patches rely on sysfs, thank you! Alex -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
On Fri, Jan 15, 2021 at 04:26:21PM +0100, Alexander Potapenko wrote: > > sysfs is "one value per file" > What about existing interfaces that even export binary blobs through > sysfs (e.g. /sys/firmware, /sys/boot_params)? > What qualifies as a "value" in that case? binary files are fine as the kernel is just a "pipe" through to the hardware / firmware. No translation or parsing happens in the kernel. And that's NOT trace events :) > > please put something like this in > > tracefs, as there is no such rules there. Or debugfs, but please, not > > sysfs. > Does tracefs have anything similar to sysfs_notify() or any other way > to implement a poll() handler? Don't know, look and see! > Our main goal is to let users wait on poll(), so that they don't have > to check the file for new contents every now and then. Is it possible > with tracefs or debugfs? debugfs, yes, you can do whatever you want. tracefs probably has this, otherwise how would trace tools work? :) > Also, for our goal debugfs isn't a particularly good fit, as Android > kernels do not enable debugfs. Do you care about Android kernels? If so, yes, debugfs is not good. And have you asked the Android kernel team about this? > Not sure about tracefs, they seem to have it, need to check. It should be there. > Do you think it is viable to keep > /sys/kernel/error_report/report_count in sysfs and use it for > notifications, but move last_report somewhere else? No, not at all, please keep it out of sysfs. > (I'd probably prefer procfs, but it could be tracefs as well, if you > find that better). If it does not have to do with processes, it does not belong in procfs. Again, this seems to fit in with tracing, so please use tracefs. > This way it will still be possible to easily notify userspace about > new reports, but the report itself won't have any atomicity > guarantees. Users will have to check the report count to ensure it > didn't change under their feet. Again, use a file in tracefs. thanks, greg k-h
On Fri, 15 Jan 2021 16:26:21 +0100 Alexander Potapenko <glider@google.com> wrote: > > please put something like this in > > tracefs, as there is no such rules there. Or debugfs, but please, not > > sysfs. > Does tracefs have anything similar to sysfs_notify() or any other way > to implement a poll() handler? > Our main goal is to let users wait on poll(), so that they don't have > to check the file for new contents every now and then. Is it possible > with tracefs or debugfs? Polling should work on tracefs. I hope it does, as I depend on it ;-) And if there's an issue, we can always add more features. > > Also, for our goal debugfs isn't a particularly good fit, as Android > kernels do not enable debugfs. > Not sure about tracefs, they seem to have it, need to check. I believe tracefs is used extensively on Android. > > Do you think it is viable to keep > /sys/kernel/error_report/report_count in sysfs and use it for > notifications, but move last_report somewhere else? > (I'd probably prefer procfs, but it could be tracefs as well, if you > find that better). If you do use tracefs, add it to the top level tracing directory (no need to have instances of it), and rename it to "kernel_warnings", as "error_report" is too close to the existing "error_log" which holds error messages of syntactic errors done by users entering in commands to some of the special files. That is, /sys/kernel/tracing/kernel_warnings/ would be your error_report/ directory you have now. Use the function in kernel/trace/trace.c: tracer_init_tracefs() to add that directory. That's for files in the tracefs directory that will not be duplicated by instances. -- Steve
> If you do use tracefs, add it to the top level tracing directory (no need > to have instances of it), and rename it to "kernel_warnings", as > "error_report" is too close to the existing "error_log" which holds error > messages of syntactic errors done by users entering in commands to some of > the special files. Will do. Is it conventional to add a new dentry* to struct trace_array for that? If not, maybe it's better to create this dir in error_report_notify_setup(), like this is done e.g. for /sys/kernel/tracing/trace_stat - just to loosen the coupling? > That is, /sys/kernel/tracing/kernel_warnings/ would be your error_report/ > directory you have now. WDYT about "kernel_errors" or "kernel_error_reports"? "warnings" suggest we'll be notifying about any occurrence of WARN(), which is not what we are planning to do. Also, shall I rename the library/config/etc. accordingly (to e.g. CONFIG_KERNEL_WARN_NOTIFY)? > Use the function in kernel/trace/trace.c: tracer_init_tracefs() to add that > directory. That's for files in the tracefs directory that will not be > duplicated by instances. > > -- Steve
On Mon, 18 Jan 2021 11:22:27 +0100 Alexander Potapenko <glider@google.com> wrote: > > If you do use tracefs, add it to the top level tracing directory (no need > > to have instances of it), and rename it to "kernel_warnings", as > > "error_report" is too close to the existing "error_log" which holds error > > messages of syntactic errors done by users entering in commands to some of > > the special files. > > Will do. Is it conventional to add a new dentry* to struct trace_array for that? > If not, maybe it's better to create this dir in > error_report_notify_setup(), like this is done e.g. for > /sys/kernel/tracing/trace_stat - just to loosen the coupling? Yeah, keep the dentry static in your own code. No need to add it to the trace_array. The dentry's in the trace array are for instances (as there are more than one of instance of them). > > > That is, /sys/kernel/tracing/kernel_warnings/ would be your error_report/ > > directory you have now. > > WDYT about "kernel_errors" or "kernel_error_reports"? > "warnings" suggest we'll be notifying about any occurrence of WARN(), > which is not what we are planning to do. > Also, shall I rename the library/config/etc. accordingly (to e.g. > CONFIG_KERNEL_WARN_NOTIFY)? I'm fine with "kernel_errors" as that helps to differentiate what they are. Also, you may want to add a field about this in the documentation under Documentation/trace/ftrace.rst (just a reference) and add a separate file called, kernel_errors.rst. -- Steve
diff --git a/Documentation/ABI/testing/sysfs-kernel-error_report b/Documentation/ABI/testing/sysfs-kernel-error_report new file mode 100644 index 000000000000..666d039f93a9 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-error_report @@ -0,0 +1,41 @@ +What: /sys/kernel/error_report/ +Date: January 2021 +Contact: Alexander Potapenko <glider@google.com>, + Marco Elver <elver@google.com> +Description: + /sys/kernel/error_report/ contains two files: "report_count" + and "last_report". These files are used to notify userspace + about error reports from the enrolled kernel subsystems (those + that use error_report_start/error_report_end tracepoints). + + "report_count" contains the current number of reported errors. + This number is incremented every time the error_report_end + trace event occurs in the kernel. + + "last_report" contains the most recent error report; concurrent + report generation results in collection of any one report + ("last_report" may not be the last shown on the console). + A "report" is everything the task had printed to the console + between issuing the error_report_start and error_report_end + trace events. + + Due to sysfs limitations, the report size is truncated at + PAGE_SIZE. To save space, the leading info in square brackets + printed by CONFIG_PRINTK_TIME and CONFIG_PRINTK_CALLER is + trimmed from the output lines. + + Both files use sysfs_notify() to notify userspace about + changes. Userspace programs can use poll() to block until an + error is reported: + + pfd.fd = fd; + pfd.events = POLLPRI; + while (1) { + lseek(pfd.fd, 0, SEEK_SET); + poll(&pfd, 1, -1); + read(pfd.fd, buffer, PAGE_SIZE); + /* Process the report in @buffer. */ + } + + Files in /sys/kernel/error_report/ are available when + CONFIG_ERROR_REPORT_NOTIFY is enabled.
Add ABI documentation for files in /sys/kernel/error_report/ Requested-by: Andrew Morton <akpm@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andrey Konovalov <andreyknvl@google.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: Marco Elver <elver@google.com> Cc: Petr Mladek <pmladek@suse.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Cc: linux-mm@kvack.org Signed-off-by: Alexander Potapenko <glider@google.com> --- .../ABI/testing/sysfs-kernel-error_report | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-error_report