Add ABI documentation for files in /sys/kernel/error_report/
Requested-by: Andrew Morton <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: [email protected]
Signed-off-by: Alexander Potapenko <[email protected]>
---
.../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 <[email protected]>,
+ Marco Elver <[email protected]>
+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.
--
2.30.0.284.gd98b1dd5eaa7-goog
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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: [email protected]
> Signed-off-by: Alexander Potapenko <[email protected]>
> ---
> .../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 <[email protected]>,
> + Marco Elver <[email protected]>
> +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 <[email protected]> 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
--
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 Mon, 18 Jan 2021 11:22:27 +0100
Alexander Potapenko <[email protected]> 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