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
>
