| Message ID | 20211015033817.16719-1-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI, APEI, EINJ: Relax platform response timeout to 1 second. | expand |
> We observe that the waiting time for DDR error injection is about 10 ms > and that for PCIe error injection is about 500 ms in Arm platfrom. > > In this patch, we relax the response timeout to 1 second and allow user to > pass the time out value as a argument. Spinning for 1ms was maybe ok. Spinning for up to 1s seems like a bad idea. This code is executed inside a mutex ... so maybe it is safe to sleep instead of spin? -Tony
Hi, Tony, Thank you for your reply. > Spinning for 1ms was maybe ok. Spinning for up to 1s seems like a bad idea. > > This code is executed inside a mutex ... so maybe it is safe to sleep instead of spin? May the email Subject misled you. This code do NOT spin for 1 sec. The period of the spinning depends on the SPIN_UNIT. > -#define SPIN_UNIT 100 /* 100ns */ > -/* Firmware should respond within 1 milliseconds */ > -#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC) > +#define SPIN_UNIT 100 /* 100us */ > +/* Firmware should respond within 1 seconds */ > +#define FIRMWARE_TIMEOUT (1 * USEC_PER_SEC) The period was 100 ns and changed to 100 us now. In my opinion, spinning for 100 ns or 100 us is OK :) The timeout_default is set with FIRMWARE_TIMEOUT (1 sec) by default. If the platform do not respond within timeout_default after multiple spins, the OSPM will print a warning message to dmesg. Regards, Shuai On 2021/10/15 PM11:37, Luck, Tony wrote: >> We observe that the waiting time for DDR error injection is about 10 ms >> and that for PCIe error injection is about 500 ms in Arm platfrom. >> >> In this patch, we relax the response timeout to 1 second and allow user to >> pass the time out value as a argument. > > Spinning for 1ms was maybe ok. Spinning for up to 1s seems like a bad idea. > > This code is executed inside a mutex ... so maybe it is safe to sleep instead of spin? > > -Tony >
On Sun, Oct 17, 2021 at 12:06:52PM +0800, Shuai Xue wrote: > Hi, Tony, > > Thank you for your reply. > > > Spinning for 1ms was maybe ok. Spinning for up to 1s seems like a bad idea. > > > > This code is executed inside a mutex ... so maybe it is safe to sleep instead of spin? > > May the email Subject misled you. This code do NOT spin for 1 sec. The period of the > spinning depends on the SPIN_UNIT. Not just the subject line. See the comment you changed here: > > -#define SPIN_UNIT 100 /* 100ns */ > > -/* Firmware should respond within 1 milliseconds */ > > -#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC) > > +#define SPIN_UNIT 100 /* 100us */ > > +/* Firmware should respond within 1 seconds */ > > +#define FIRMWARE_TIMEOUT (1 * USEC_PER_SEC) That definitely reads to me that the timeout was increased from 1 millisecond to 1 second. With the old code polling for completion every 100ns, and the new code polling every 100us > > The period was 100 ns and changed to 100 us now. In my opinion, spinning for 100 ns or 100 us is OK :) But what does the code do in between polls? The calling code is: for (;;) { rc = apei_exec_run(&ctx, ACPI_EINJ_CHECK_BUSY_STATUS); if (rc) return rc; val = apei_exec_ctx_get_output(&ctx); if (!(val & EINJ_OP_BUSY)) break; if (einj_timedout(&timeout)) return -EIO; } Now apei_exec_run() and apei_exec_ctx_get_output() are a maze of functions & macros. But I don't think they can block, sleep, or context switch. So this code is "spinning" until either BIOS says the operation is complete, or the FIRMWARE_TIMEOUT is reached. It avoids triggering a watchdog by the call to touch_nmi_watchdog() after each spin between polls. But the whole thing may be spinning for a second. I'm not at all sure that I'm right that the spin could be replaced with an msleep(). It will certainly slow things down for systems and EINJ operations that actually complete quickly (because instead of returnining within 100ns (or 100us with your patch) it will sleep for 1 ms (rounded up to next jiffie ... so 4 ms of HZ=250 systems. But I don't care if my error injections take 4ms. I do care that one logical CPU spins for 1 second. -Tony
Hi Tony, > I'm not at all sure that I'm right that the spin could be replaced > with an msleep(). It will certainly slow things down for systems > and EINJ operations that actually complete quickly (because instead > of returnining within 100ns (or 100us with your patch) it will sleep > for 1 ms (rounded up to next jiffie ... so 4 ms of HZ=250 systems. > > But I don't care if my error injections take 4ms. > > I do care that one logical CPU spins for 1 second. Agree. The side effect of sleep is to slow down the injection that actually complete quickly and error injection is not concerned with real-time. I will send a v2 patch implemented in msleep soon. Regards. Shuai On 2021/10/18 PM11:40, Luck, Tony wrote: > On Sun, Oct 17, 2021 at 12:06:52PM +0800, Shuai Xue wrote: >> Hi, Tony, >> >> Thank you for your reply. >> >>> Spinning for 1ms was maybe ok. Spinning for up to 1s seems like a bad idea. >>> >>> This code is executed inside a mutex ... so maybe it is safe to sleep instead of spin? >> >> May the email Subject misled you. This code do NOT spin for 1 sec. The period of the >> spinning depends on the SPIN_UNIT. > > Not just the subject line. See the comment you changed here: > >>> -#define SPIN_UNIT 100 /* 100ns */ >>> -/* Firmware should respond within 1 milliseconds */ >>> -#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC) >>> +#define SPIN_UNIT 100 /* 100us */ >>> +/* Firmware should respond within 1 seconds */ >>> +#define FIRMWARE_TIMEOUT (1 * USEC_PER_SEC) > > That definitely reads to me that the timeout was increased from > 1 millisecond to 1 second. With the old code polling for completion > every 100ns, and the new code polling every 100us >> >> The period was 100 ns and changed to 100 us now. In my opinion, spinning for 100 ns or 100 us is OK :) > > But what does the code do in between polls? The calling code is: > > for (;;) { > rc = apei_exec_run(&ctx, ACPI_EINJ_CHECK_BUSY_STATUS); > if (rc) > return rc; > val = apei_exec_ctx_get_output(&ctx); > if (!(val & EINJ_OP_BUSY)) > break; > if (einj_timedout(&timeout)) > return -EIO; > } > > Now apei_exec_run() and apei_exec_ctx_get_output() are a maze of > functions & macros. But I don't think they can block, sleep, or > context switch. > > So this code is "spinning" until either BIOS says the operation is > complete, or the FIRMWARE_TIMEOUT is reached. > > It avoids triggering a watchdog by the call to touch_nmi_watchdog() > after each spin between polls. But the whole thing may be spinning > for a second. > > I'm not at all sure that I'm right that the spin could be replaced > with an msleep(). It will certainly slow things down for systems > and EINJ operations that actually complete quickly (because instead > of returnining within 100ns (or 100us with your patch) it will sleep > for 1 ms (rounded up to next jiffie ... so 4 ms of HZ=250 systems. > > But I don't care if my error injections take 4ms. > > I do care that one logical CPU spins for 1 second. > > -Tony >
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c index 133156759551..fa2386ee37db 100644 --- a/drivers/acpi/apei/einj.c +++ b/drivers/acpi/apei/einj.c @@ -14,6 +14,7 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/moduleparam.h> #include <linux/init.h> #include <linux/io.h> #include <linux/debugfs.h> @@ -28,9 +29,9 @@ #undef pr_fmt #define pr_fmt(fmt) "EINJ: " fmt -#define SPIN_UNIT 100 /* 100ns */ -/* Firmware should respond within 1 milliseconds */ -#define FIRMWARE_TIMEOUT (1 * NSEC_PER_MSEC) +#define SPIN_UNIT 100 /* 100us */ +/* Firmware should respond within 1 seconds */ +#define FIRMWARE_TIMEOUT (1 * USEC_PER_SEC) #define ACPI5_VENDOR_BIT BIT(31) #define MEM_ERROR_MASK (ACPI_EINJ_MEMORY_CORRECTABLE | \ ACPI_EINJ_MEMORY_UNCORRECTABLE | \ @@ -40,6 +41,8 @@ * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action. */ static int acpi5; +static int timeout_default = FIRMWARE_TIMEOUT; +module_param(timeout_default, int, 0644); struct set_error_type_with_address { u32 type; @@ -176,7 +179,7 @@ static int einj_timedout(u64 *t) return 1; } *t -= SPIN_UNIT; - ndelay(SPIN_UNIT); + udelay(SPIN_UNIT); touch_nmi_watchdog(); return 0; } @@ -403,7 +406,7 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3, u64 param4) { struct apei_exec_context ctx; - u64 val, trigger_paddr, timeout = FIRMWARE_TIMEOUT; + u64 val, trigger_paddr, timeout = timeout_default; int rc; einj_exec_ctx_init(&ctx);
When injecting an error into the platform, the OSPM executes an EXECUTE_OPERATION action to instruct the platform to begin the injection operation. And then, the OSPM busy waits for a while by continually executing CHECK_BUSY_STATUS action until the platform indicates that the operation is complete. More specifically, the platform is limited to respond within 1 millisecond right now. This is too strict for some platforms. For example, in Arm platfrom, when injecting a Processor Correctable error, the OSPM will warn: Firmware does not respond in time. And a message is printed on the console: echo: write error: Input/output error We observe that the waiting time for DDR error injection is about 10 ms and that for PCIe error injection is about 500 ms in Arm platfrom. In this patch, we relax the response timeout to 1 second and allow user to pass the time out value as a argument. Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> --- drivers/acpi/apei/einj.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)