Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
payloads/ext/Makefile.inc: Fix SeaBIOS race condition
For a very long time, SeaBIOS sometimes failed to build when using multiple threads. This known problem has been haunting everyone for a very long time. Until now.
Unlike most other payloads, building SeaBIOS results in two files: the SeaBIOS payload itself and SeaVGABIOS. Each file has its own target, and there's a third target called "seabios", which has the same recipe as the SeaBIOS file, which calls `payloads/external/SeaBIOS/Makefile` with a bunch of arguments. In addition, SeaVGABIOS depends on "seabios".
When executing serially, if the file of either SeaBIOS or SeaVGABIOS is needed, the SeaBIOS Makefile will be run. This will generate both files, so it is not necessary to run the Makefile more than once.
However, when using multiple threads, it can happen that one thread wants to make the SeaBIOS file, while another one wants to make the SeaVGABIOS file, which depends on "seabios". This implies that both threads will execute the SeaBIOS Makefile at about the same time, only to collide when performing git operations. Since git uses a lock file when updating the index, one of the threads will fail to acquire the lock with an error, which will ultimately cause the build to fail.
Whenever this happened, manually aborting with Ctrl-C made the build process fail again because of the same error. The only way to get past this problem, other than using one thread, was to let the unfinished jobs complete. The thread that acquired the lock on the SeaBIOS git repository would finish building SeaBIOS, so that target would not need to be remade. When restarting the build, only the target that failed is rebuilt, so it does not collide with any other thread.
To address this issue, make the SeaVGABIOS file target depend directly on the SeaBIOS file instead, and remove the duplicate "seabios" target.
Change-Id: I251190d3bb27052ff474f3cd1a45022dab6fac31 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M payloads/external/Makefile.inc 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/39188/1
diff --git a/payloads/external/Makefile.inc b/payloads/external/Makefile.inc index b8af8c9..0a96aff 100644 --- a/payloads/external/Makefile.inc +++ b/payloads/external/Makefile.inc @@ -78,7 +78,7 @@ # SeaBIOS
SEABIOS_CC_OFFSET=$(if $(filter %ccache,$(HOSTCC)),2,1) -payloads/external/SeaBIOS/seabios/out/bios.bin.elf seabios: $(DOTCONFIG) +payloads/external/SeaBIOS/seabios/out/bios.bin.elf: $(DOTCONFIG) $(MAKE) -C payloads/external/SeaBIOS \ HOSTCC="$(HOSTCC)" \ CC=$(word $(SEABIOS_CC_OFFSET),$(CC_x86_32)) \ @@ -104,7 +104,7 @@ CONFIG_ENABLE_HSUART=$(CONFIG_ENABLE_HSUART) \ CONFIG_CONSOLE_UART_BASE_ADDRESS=$(CONFIG_CONSOLE_UART_BASE_ADDRESS)
-payloads/external/SeaBIOS/seabios/out/vgabios.bin: seabios +payloads/external/SeaBIOS/seabios/out/vgabios.bin: payloads/external/SeaBIOS/seabios/out/bios.bin.elf payloads/external/SeaBIOS/seabios/.config: payloads/external/SeaBIOS/seabios/out/bios.bin.elf payloads/external/SeaBIOS/seabios/out/autoversion.h: payloads/external/SeaBIOS/seabios/out/bios.bin.elf
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
Patch Set 1: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
Patch Set 1:
This does remove the seabios target (which probably should be a phony).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
Patch Set 1:
Patch Set 1:
This does remove the seabios target (which probably should be a phony).
Uh, "the" `seabios` target? There are (at least) two `seabios` targets: one is removed here, and another one in `payloads/external/SeaBIOS/Makefile`. The latter is what actually clones the repo into the `seabios` folder.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
payloads/ext/Makefile.inc: Fix SeaBIOS race condition
For a very long time, SeaBIOS sometimes failed to build when using multiple threads. This known problem has been haunting everyone for a very long time. Until now.
Unlike most other payloads, building SeaBIOS results in two files: the SeaBIOS payload itself and SeaVGABIOS. Each file has its own target, and there's a third target called "seabios", which has the same recipe as the SeaBIOS file, which calls `payloads/external/SeaBIOS/Makefile` with a bunch of arguments. In addition, SeaVGABIOS depends on "seabios".
When executing serially, if the file of either SeaBIOS or SeaVGABIOS is needed, the SeaBIOS Makefile will be run. This will generate both files, so it is not necessary to run the Makefile more than once.
However, when using multiple threads, it can happen that one thread wants to make the SeaBIOS file, while another one wants to make the SeaVGABIOS file, which depends on "seabios". This implies that both threads will execute the SeaBIOS Makefile at about the same time, only to collide when performing git operations. Since git uses a lock file when updating the index, one of the threads will fail to acquire the lock with an error, which will ultimately cause the build to fail.
Whenever this happened, manually aborting with Ctrl-C made the build process fail again because of the same error. The only way to get past this problem, other than using one thread, was to let the unfinished jobs complete. The thread that acquired the lock on the SeaBIOS git repository would finish building SeaBIOS, so that target would not need to be remade. When restarting the build, only the target that failed is rebuilt, so it does not collide with any other thread.
To address this issue, make the SeaVGABIOS file target depend directly on the SeaBIOS file instead, and remove the duplicate "seabios" target.
Change-Id: I251190d3bb27052ff474f3cd1a45022dab6fac31 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39188 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Nico Huber nico.h@gmx.de --- M payloads/external/Makefile.inc 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Arthur Heymans: Looks good to me, but someone else must approve
diff --git a/payloads/external/Makefile.inc b/payloads/external/Makefile.inc index b8af8c9..0a96aff 100644 --- a/payloads/external/Makefile.inc +++ b/payloads/external/Makefile.inc @@ -78,7 +78,7 @@ # SeaBIOS
SEABIOS_CC_OFFSET=$(if $(filter %ccache,$(HOSTCC)),2,1) -payloads/external/SeaBIOS/seabios/out/bios.bin.elf seabios: $(DOTCONFIG) +payloads/external/SeaBIOS/seabios/out/bios.bin.elf: $(DOTCONFIG) $(MAKE) -C payloads/external/SeaBIOS \ HOSTCC="$(HOSTCC)" \ CC=$(word $(SEABIOS_CC_OFFSET),$(CC_x86_32)) \ @@ -104,7 +104,7 @@ CONFIG_ENABLE_HSUART=$(CONFIG_ENABLE_HSUART) \ CONFIG_CONSOLE_UART_BASE_ADDRESS=$(CONFIG_CONSOLE_UART_BASE_ADDRESS)
-payloads/external/SeaBIOS/seabios/out/vgabios.bin: seabios +payloads/external/SeaBIOS/seabios/out/vgabios.bin: payloads/external/SeaBIOS/seabios/out/bios.bin.elf payloads/external/SeaBIOS/seabios/.config: payloads/external/SeaBIOS/seabios/out/bios.bin.elf payloads/external/SeaBIOS/seabios/out/autoversion.h: payloads/external/SeaBIOS/seabios/out/bios.bin.elf
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39188 )
Change subject: payloads/ext/Makefile.inc: Fix SeaBIOS race condition ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1059 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1058 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1057
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)