On Fri, Nov 19, 2010 at 9:56 PM, <boris@xxxxxxxxx> wrote: > Yes, we need a generic error reporting format. Wait a second, this > error format structure looks very much like a tracepoint record to me - > it has common fields and record-specific fields. And we have all that > infrastructure in the kernel and yet you've decided to reimplement it > anew. Remind me again why? You mean "struct trace_entry"? They are quite different on design. The record format in patch can incorporate multiple sections into one record, which is meaningful for hardware error reporting. And we do not need the fancy "/sys/kernel/debug/tracing/events/<xxx>/<xxx>/format", user space error daemon only consumes all error record it recognized and blindly log all other records. >> The hardware error reporting mechanism designed by the patch >> integrates well with device model in kernel. Âstruct dev_herr_info is >> defined and pointed to by "error" field of struct device. ÂThis is >> used to hold error reporting related information for each device. ÂOne >> sysfs directory "error" will be created for each hardware error >> reporting device. ÂSome files for error reporting statistics and >> control are created in sysfs "error" directory. > > But all this APEI crap sounds suspiciously bloated There is no APEI specific code in this patch. > - why do we need an > error field for _every_ device on the system? Looks like a bunch of > hoopla for only a few error types... Because every device may report hardware errors, but not every device will do it. So just a pointer is added to "struct device" and corresponding data structure is only created when needed. >> For example, the >> "error" directory for APEI GHES is as follow. >> >> /sys/devices/platform/GHES.0/error/logs >> /sys/devices/platform/GHES.0/error/overflows >> /sys/devices/platform/GHES.0/error/throttles >> >> Where "logs" is number of error records logged; "throttles" is number >> of error records not logged because the reporting rate is too high; >> "overflows" is number of error records not logged because there is no >> space available. >> >> Not all devices will report errors, so struct dev_herr_info and sysfs >> directory/files are only allocated/created for devices explicitly >> enable it. ÂSo to enumerate the error sources of system, you just need >> to enumerate "error" directory for each device directory in >> /sys/devices. > > So how do you say which devices should report and which shouldn't report > errors, from userspace with a tool? Default actions? What if you forget > to enable error reporting of a device which actually is important? In general all hardware errors should be reported and logged. >> One device file (/dev/error/error) which mixed error records from all >> hardware error reporting devices is created to convey error records >> from kernel space to user space. ÂBecause hardware devices are dealt >> with, a device file is the most natural way to do that. ÂBecause >> hardware error reporting should not hurts system performance, the >> throughput of the interface should be controlled to a low level (done >> by user space error daemon), ordinary "read" is sufficient from >> performance point of view. > > Right, so no need for a daemon but who does the read? cat? How are you > going to collect all those errors? How do you enforce policies? Some summary hardware error information can be put into printk. Error daemon is needed because we need not only log the the error but the predictive recovery. If you really have no daemon, cat can be used to log the error. I don't fully understand your words, you want to enforce policies without error daemon? > How do you inject errors? We can use another device file to inject error, for example /dev/error/error_inject. Just write the needed information to this file. The format can be same as the error record defined as above, because it is highly extensible. > How do you perform actions based on the error > type like disabling or reconfiguring a hw device based on the errors it > generates? These are policies and will be done in user space error daemon. For some emergency error recovery actions, we will do it in kernel. >> The patch provides common services for hardware error reporting >> devices too. >> >> A lock-less hardware error record allocator is provided. ÂSo for >> hardware error that can be ignored (such as corrected errors), it is >> not needed to pre-allocate the error record or allocate the error >> record on stack. ÂBecause the possibility for two hardware parts to go >> error simultaneously is very small, one big unified memory pool for >> hardware errors is better than one memory pool or buffer for each >> device. > > Yet another bloat winner. Why do we need a memory allocator for error > records? The point is lockless not the memory allocator. The lockless memory allocator is not hardware error reporting specific, it can be used by other part of the kernel too. > You either get a single critical error which shuts down the > system and you log it to persistent storage, if possible, or you work at > those uncritical errors one at a time. Uncritical errors can be reported in NMI handler too. So we need lockless data structure for them. > IOW, do you have a real-life usecase which justifies the dire need for a > lockless memory allocator specifically for hw errors? No. not special for hw errors. It can be used by other part of kernel. >> After filling in all necessary fields in hardware error record, the >> error reporting is quite straightforward, just calling >> herr_record_report, parameters are the error record itself and the >> corresponding struct device. >> >> Hardware errors may burst, for example, same hardware errors may be >> reported at high rate within a short interval, this will use up all >> pre-allocated memory for error reporting, so that other hardware >> errors come from same or different hardware device can not be logged. >> To deal with this issue, a throttle algorithm is implemented. ÂThe >> logging rate for errors come from one hardware error device is >> throttled based on the available pre-allocated memory for error >> reporting. ÂIn this way we can log as many kinds of errors as possible >> comes from as many devices as possible. >> >> >> This patch is designed by Andi Kleen and Huang Ying. >> >> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx> >> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx> >> --- >> Âdrivers/Kconfig        |  Â2 >> Âdrivers/Makefile       Â|  Â1 >> Âdrivers/base/Makefile     |  Â1 >> Âdrivers/base/herror.c     |  98 ++++++++ >> Âdrivers/herror/Kconfig    Â|  Â5 >> Âdrivers/herror/Makefile    |  Â1 >> Âdrivers/herror/herr-core.c  Â| Â488 > ++++++++++++++++++++++++++++++++++++++++++ >> Âinclude/linux/Kbuild     Â|  Â1 >> Âinclude/linux/device.h    Â|  14 + >> Âinclude/linux/herror.h    Â|  35 +++ >> Âinclude/linux/herror_record.h | Â100 ++++++++ >> Âkernel/Makefile        |  Â1 >> Â12 files changed, 747 insertions(+) >> Âcreate mode 100644 drivers/base/herror.c >> Âcreate mode 100644 drivers/herror/Kconfig >> Âcreate mode 100644 drivers/herror/Makefile >> Âcreate mode 100644 drivers/herror/herr-core.c >> Âcreate mode 100644 include/linux/herror.h >> Âcreate mode 100644 include/linux/herror_record.h > > And as it was said a countless times already, this whole thing, if at > all accepted should go to drivers/edac/ or drivers/ras/ or whatever. You think drivers/herror is not a good name? We can rename it to "drivers/ras" if that is the consensus. Best Regards, Huang Ying -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
