[PATCH] Disable timer check in file loading

[PATCH] Disable timer check in file loading

[Michael Chang]

The u-boot efi console service registers a timer to poll the keyboard
input in every 50ns. In the efi block io service, this timer is
evaluated on each block read, and since the timer interval is much less
than the time needed to reading out a block (32kB) from the disk, the
keyboard polling is therefore in the wake of each block read.

Unfortunately USB keyboard spends too much time in polling. In my test
usb_kbd_poll_for_event costs 40ms in usb_kbd_testc() to test if a
character is in the queue. In combination with the number of blocks to
be read from the disk, the extra amound of time delayed could be around
30 seconds to load linux and initrd.

For that matters, the timer check is disabled in file loading to speed
it up. The consequence would be losing the keystroke during the time
file is loading, but that is acceptable IMHO.

Downstream bug reference:
https://bugzilla.suse.com/show_bug.cgi?id=1171222

Signed-off-by: Michael Chang <mchang@suse.com>
---
 lib/efi_loader/efi_disk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 307d5d759b..d090110b06 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -15,6 +15,7 @@
 #include <log.h>
 #include <part.h>
 #include <malloc.h>
+#include <watchdog.h>
 
 struct efi_system_partition efi_system_partition;
 
@@ -103,8 +104,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 	else
 		n = blk_dwrite(desc, lba, blocks, buffer);
 
-	/* We don't do interrupts, so check for timers cooperatively */
-	efi_timer_check();
+	WATCHDOG_RESET();
 
 	EFI_PRINT("n=%lx blocks=%x\n", n, blocks);
 
-- 
2.26.2

Re: [PATCH] Disable timer check in file loading

[Heinrich Schuchardt]

On 7/8/21 10:22 AM, Michael Chang wrote:
> The u-boot efi console service registers a timer to poll the keyboard
> input in every 50ns. In the efi block io service, this timer is

The event is triggered every 5000 ns not 50 ns. See
lib/efi_loader/efi_console.c:1309. The SetTimer() parameter is in
multiples of 100 ns. UEFI spec 2.9 has this sentence:
"TimerRelative: The event is to be signaled in TriggerTime 100ns units."

> evaluated on each block read, and since the timer interval is much less
> than the time needed to reading out a block (32kB) from the disk, the
> keyboard polling is therefore in the wake of each block read.
>
> Unfortunately USB keyboard spends too much time in polling. In my test
> usb_kbd_poll_for_event costs 40ms in usb_kbd_testc() to test if a

I can't imagine that Linux is that slow. Why is U-Boot so slow? Please,
try to fix that code.

> character is in the queue. In combination with the number of blocks to
> be read from the disk, the extra amound of time delayed could be around
> 30 seconds to load linux and initrd.
>
> For that matters, the timer check is disabled in file loading to speed
> it up. The consequence would be losing the keystroke during the time
> file is loading, but that is acceptable IMHO.

Disabling would mean that programs like iPXE cannot react to keyboard
input to interrupt a file transfer.

Best regards

Heinrich

>
> Downstream bug reference:
> https://bugzilla.suse.com/show_bug.cgi?id=1171222
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>   lib/efi_loader/efi_disk.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 307d5d759b..d090110b06 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -15,6 +15,7 @@
>   #include <log.h>
>   #include <part.h>
>   #include <malloc.h>
> +#include <watchdog.h>
>
>   struct efi_system_partition efi_system_partition;
>
> @@ -103,8 +104,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
>   	else
>   		n = blk_dwrite(desc, lba, blocks, buffer);
>
> -	/* We don't do interrupts, so check for timers cooperatively */
> -	efi_timer_check();
> +	WATCHDOG_RESET();
>
>   	EFI_PRINT("n=%lx blocks=%x\n", n, blocks);
>
>

Re: [PATCH] Disable timer check in file loading

[Michael Chang]

On Thu, Jul 15, 2021 at 04:51:35PM +0200, Heinrich Schuchardt wrote:
> On 7/8/21 10:22 AM, Michael Chang wrote:
> > The u-boot efi console service registers a timer to poll the keyboard
> > input in every 50ns. In the efi block io service, this timer is
> 
> The event is triggered every 5000 ns not 50 ns. See
> lib/efi_loader/efi_console.c:1309. The SetTimer() parameter is in
> multiples of 100 ns. UEFI spec 2.9 has this sentence:
> "TimerRelative: The event is to be signaled in TriggerTime 100ns units."

Indeed the value is mistakenly taken. Thanks to pointing this out and will
correct that in followed version. 

> > evaluated on each block read, and since the timer interval is much less
> > than the time needed to reading out a block (32kB) from the disk, the
> > keyboard polling is therefore in the wake of each block read.
> > 
> > Unfortunately USB keyboard spends too much time in polling. In my test
> > usb_kbd_poll_for_event costs 40ms in usb_kbd_testc() to test if a
> 
> I can't imagine that Linux is that slow. Why is U-Boot so slow? Please,
> try to fix that code.

Well I'm by no means a usb expert who is able to troubleshooting this
myself so disabling the timer check is all I can do to mitigate the
problem firstly.

I certainly agree to investigate into usb_kbd_testc to understand what's
happening under the hood, but please bear with me if that takes longer
than expected to provide a solution/expaination along the way. For that
I would have to seek for help internally from the company.

> > character is in the queue. In combination with the number of blocks to
> > be read from the disk, the extra amound of time delayed could be around
> > 30 seconds to load linux and initrd.
> > 
> > For that matters, the timer check is disabled in file loading to speed
> > it up. The consequence would be losing the keystroke during the time
> > file is loading, but that is acceptable IMHO.
> 
> Disabling would mean that programs like iPXE cannot react to keyboard
> input to interrupt a file transfer.

Ok. fair enough.

Thanks a lot for review.

Regards,
Michael

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Downstream bug reference:
> > https://bugzilla.suse.com/show_bug.cgi?id=1171222
> > 
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >   lib/efi_loader/efi_disk.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 307d5d759b..d090110b06 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -15,6 +15,7 @@
> >   #include <log.h>
> >   #include <part.h>
> >   #include <malloc.h>
> > +#include <watchdog.h>
> > 
> >   struct efi_system_partition efi_system_partition;
> > 
> > @@ -103,8 +104,7 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
> >   	else
> >   		n = blk_dwrite(desc, lba, blocks, buffer);
> > 
> > -	/* We don't do interrupts, so check for timers cooperatively */
> > -	efi_timer_check();
> > +	WATCHDOG_RESET();
> > 
> >   	EFI_PRINT("n=%lx blocks=%x\n", n, blocks);
> > 
> > 
>