[1/1] core/pkg-infra: Fix package files statistics for parallel build

Message ID 20190305144216.13738-1-anaumann@ultratronik.de
State New
Headers show
Series
  • [1/1] core/pkg-infra: Fix package files statistics for parallel build
Related show

Commit Message

Andreas Naumann March 5, 2019, 2:42 p.m.
When activating top level parallel build, multiple processes may
try to modify the various *files-list* files in the common build/
directory at the same time. This can cause racy build failures.

The fix here is to use flock to ensure exclusive execution of the
statistics gathering code. For this to work it is assumed that the
target/staging/host directories are isolated, which is true for
per-package builds.
For standard sequential builds, the locking is of course unnecessary
but no conditional handling is implemented  since the runtime cost is
expected to be next to nothing.

For flock to work, the statistics gathering code must be run in a
single subshell. Otherwise the lockfile descriptor would be closed
after the subshell (= the Makefile line where it was opened) ends
and thus the lock would be released before even entering the critical
code section.

Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
---

Notes:
This patch has been developed on top of Thomas ppsh-v7 branch, rebased
on master.

Of course this solution adds a dependency on flock. However flock is
part of util-linux which probably is a dependency of other buildroot
prerequisites anyway.

Also, even though this solution has a somewhat hacky feel to me, it does
work for both per-package and standard sequential builds. When converting
to per-package for good, the statistics gathering code could probably
be simplified because, well, all the files are installed isolated folders
which contents should be easier to compare before/after.

 package/pkg-generic.mk | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Yann E. MORIN March 5, 2019, 3:39 p.m. | #1
Andreas, All,

On 2019-03-05 15:42 +0100, Andreas Naumann spake thusly:
> When activating top level parallel build, multiple processes may
> try to modify the various *files-list* files in the common build/
> directory at the same time. This can cause racy build failures.
> 
> The fix here is to use flock to ensure exclusive execution of the
> statistics gathering code. For this to work it is assumed that the
> target/staging/host directories are isolated, which is true for
> per-package builds.
> For standard sequential builds, the locking is of course unnecessary
> but no conditional handling is implemented  since the runtime cost is
> expected to be next to nothing.
> 
> For flock to work, the statistics gathering code must be run in a
> single subshell. Otherwise the lockfile descriptor would be closed
> after the subshell (= the Makefile line where it was opened) ends
> and thus the lock would be released before even entering the critical
> code section.
> 
> Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
> ---
> 
> Notes:
> This patch has been developed on top of Thomas ppsh-v7 branch, rebased
> on master.
> 
> Of course this solution adds a dependency on flock. However flock is
> part of util-linux which probably is a dependency of other buildroot
> prerequisites anyway.

We already use flock in the download infra, so that's OK.

> Also, even though this solution has a somewhat hacky feel to me, it does
> work for both per-package and standard sequential builds. When converting
> to per-package for good, the statistics gathering code could probably
> be simplified because, well, all the files are installed isolated folders
> which contents should be easier to compare before/after.

Well, I was going to suggest just that: create the lists in the
per-package directory, so you won't have the race to begin with.

Then, at the end of the build, when all the target/ directories (ditto
host) are gathere into one, then do so for all the individual package
file lists too.

If, as you seem to suggest, this also simplifies the code, then bonus
point for you! ;-)

Regards,
Yann E. MORIN.

>  package/pkg-generic.mk | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 6d4442b824..0f875736d6 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -64,20 +64,22 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time
>  # $(3): suffix of file  (optional)
>  define step_pkg_size_inner
>  	@touch $(BUILD_DIR)/.files-list$(3).stat
> -	@touch $(BUILD_DIR)/packages-file-list$(3).txt
> -	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt
> +	exec 3>$(BUILD_DIR)/packages-file-list$(3).txt; \
> +	flock -x 3; \
> +	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt; \
>  	cd $(2); \
>  	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> -		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new
> +		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new; \
>  	LC_ALL=C comm -13 \
>  		$(BUILD_DIR)/.files-list$(3).stat \
>  		$(BUILD_DIR)/.files-list$(3).new \
> -		> $($(PKG)_BUILDDIR)/.files-list$(3).txt
> +		> $($(PKG)_BUILDDIR)/.files-list$(3).txt; \
>  	sed -r -e 's/^[^,]+/$(1)/' \
>  		$($(PKG)_BUILDDIR)/.files-list$(3).txt \
> -		>> $(BUILD_DIR)/packages-file-list$(3).txt
> +		>> $(BUILD_DIR)/packages-file-list$(3).txt; \
>  	mv $(BUILD_DIR)/.files-list$(3).new \
> -		$(BUILD_DIR)/.files-list$(3).stat
> +		$(BUILD_DIR)/.files-list$(3).stat; \
> +	exec 3>&-
>  endef
>  
>  define step_pkg_size
> -- 
> 2.21.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Andreas Naumann March 5, 2019, 6:51 p.m. | #2
Hi Yann,

...

>>
>> Of course this solution adds a dependency on flock. However flock is
>> part of util-linux which probably is a dependency of other buildroot
>> prerequisites anyway.
> 
> We already use flock in the download infra, so that's OK.

Ah, nice.

>> Also, even though this solution has a somewhat hacky feel to me, it does
>> work for both per-package and standard sequential builds. When converting
>> to per-package for good, the statistics gathering code could probably
>> be simplified because, well, all the files are installed isolated folders
>> which contents should be easier to compare before/after.
> 
> Well, I was going to suggest just that: create the lists in the
> per-package directory, so you won't have the race to begin with.
> 
> Then, at the end of the build, when all the target/ directories (ditto
> host) are gathere into one, then do so for all the individual package
> file lists too.

Yes I was going to do that but realized it would lead to different code 
for the same thing. And as far as I understood, the per-package stuff is 
planned as an option. Maintaining two implementations felt less ideal, 
even if one of them turns out more straightforward (which has yet to be 
proven).

> 
> If, as you seem to suggest, this also simplifies the code, then bonus
> point for you! ;-)

Well this was theory, I havn't really thought it out yet. As I wrote 
above, I'd leave that until per-package is merged and proves stable.


best regards,
Andreas



> 
> Regards,
> Yann E. MORIN.
> 
>>   package/pkg-generic.mk | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index 6d4442b824..0f875736d6 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -64,20 +64,22 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time
>>   # $(3): suffix of file  (optional)
>>   define step_pkg_size_inner
>>   	@touch $(BUILD_DIR)/.files-list$(3).stat
>> -	@touch $(BUILD_DIR)/packages-file-list$(3).txt
>> -	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt
>> +	exec 3>$(BUILD_DIR)/packages-file-list$(3).txt; \
>> +	flock -x 3; \
>> +	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt; \
>>   	cd $(2); \
>>   	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
>> -		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new
>> +		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new; \
>>   	LC_ALL=C comm -13 \
>>   		$(BUILD_DIR)/.files-list$(3).stat \
>>   		$(BUILD_DIR)/.files-list$(3).new \
>> -		> $($(PKG)_BUILDDIR)/.files-list$(3).txt
>> +		> $($(PKG)_BUILDDIR)/.files-list$(3).txt; \
>>   	sed -r -e 's/^[^,]+/$(1)/' \
>>   		$($(PKG)_BUILDDIR)/.files-list$(3).txt \
>> -		>> $(BUILD_DIR)/packages-file-list$(3).txt
>> +		>> $(BUILD_DIR)/packages-file-list$(3).txt; \
>>   	mv $(BUILD_DIR)/.files-list$(3).new \
>> -		$(BUILD_DIR)/.files-list$(3).stat
>> +		$(BUILD_DIR)/.files-list$(3).stat; \
>> +	exec 3>&-
>>   endef
>>   
>>   define step_pkg_size
>> -- 
>> 2.21.0
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>
Jan Kundrát March 7, 2019, 12:36 p.m. | #3
On úterý 5. března 2019 15:42:16 CET, Andreas Naumann wrote:
> When activating top level parallel build, multiple processes may
> try to modify the various *files-list* files in the common build/
> directory at the same time. This can cause racy build failures.
>
> The fix here is to use flock to ensure exclusive execution of the
> statistics gathering code. For this to work it is assumed that the
> target/staging/host directories are isolated, which is true for
> per-package builds.
> For standard sequential builds, the locking is of course unnecessary
> but no conditional handling is implemented  since the runtime cost is
> expected to be next to nothing.
>
> For flock to work, the statistics gathering code must be run in a
> single subshell. Otherwise the lockfile descriptor would be closed
> after the subshell (= the Makefile line where it was opened) ends
> and thus the lock would be released before even entering the critical
> code section.
>
> Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
> ---
>
> Notes:
> This patch has been developed on top of Thomas ppsh-v7 branch, rebased
> on master.
>
> Of course this solution adds a dependency on flock. However flock is
> part of util-linux which probably is a dependency of other buildroot
> prerequisites anyway.
>
> Also, even though this solution has a somewhat hacky feel to me, it does
> work for both per-package and standard sequential builds. When converting
> to per-package for good, the statistics gathering code could probably
> be simplified because, well, all the files are installed isolated folders
> which contents should be easier to compare before/after.
>
>  package/pkg-generic.mk | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 6d4442b824..0f875736d6 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -64,20 +64,22 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time
>  # $(3): suffix of file  (optional)
>  define step_pkg_size_inner
>  	@touch $(BUILD_DIR)/.files-list$(3).stat
> -	@touch $(BUILD_DIR)/packages-file-list$(3).txt
> -	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt
> +	exec 3>$(BUILD_DIR)/packages-file-list$(3).txt; \
> +	flock -x 3; \
> +	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt; \
>  	cd $(2); \
>  	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> -		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new
> +		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new; \
>  	LC_ALL=C comm -13 \
>  		$(BUILD_DIR)/.files-list$(3).stat \
>  		$(BUILD_DIR)/.files-list$(3).new \
> -		> $($(PKG)_BUILDDIR)/.files-list$(3).txt
> +		> $($(PKG)_BUILDDIR)/.files-list$(3).txt; \
>  	sed -r -e 's/^[^,]+/$(1)/' \
>  		$($(PKG)_BUILDDIR)/.files-list$(3).txt \
> -		>> $(BUILD_DIR)/packages-file-list$(3).txt
> +		>> $(BUILD_DIR)/packages-file-list$(3).txt; \
>  	mv $(BUILD_DIR)/.files-list$(3).new \
> -		$(BUILD_DIR)/.files-list$(3).stat
> +		$(BUILD_DIR)/.files-list$(3).stat; \
> +	exec 3>&-
>  endef
>  
>  define step_pkg_size

Thanks, I can confirm that this works on my setup.

Tested-by: Jan Kundrát <jan.kundrat@cesnet.cz>
Fixes: 3c8f0d9efa (core/pkg-infra: restore completeness of packages files 
lists)

Patch

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 6d4442b824..0f875736d6 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -64,20 +64,22 @@  GLOBAL_INSTRUMENTATION_HOOKS += step_time
 # $(3): suffix of file  (optional)
 define step_pkg_size_inner
 	@touch $(BUILD_DIR)/.files-list$(3).stat
-	@touch $(BUILD_DIR)/packages-file-list$(3).txt
-	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt
+	exec 3>$(BUILD_DIR)/packages-file-list$(3).txt; \
+	flock -x 3; \
+	$(SED) '/^$(1),/d' $(BUILD_DIR)/packages-file-list$(3).txt; \
 	cd $(2); \
 	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
-		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new
+		| LC_ALL=C sort > $(BUILD_DIR)/.files-list$(3).new; \
 	LC_ALL=C comm -13 \
 		$(BUILD_DIR)/.files-list$(3).stat \
 		$(BUILD_DIR)/.files-list$(3).new \
-		> $($(PKG)_BUILDDIR)/.files-list$(3).txt
+		> $($(PKG)_BUILDDIR)/.files-list$(3).txt; \
 	sed -r -e 's/^[^,]+/$(1)/' \
 		$($(PKG)_BUILDDIR)/.files-list$(3).txt \
-		>> $(BUILD_DIR)/packages-file-list$(3).txt
+		>> $(BUILD_DIR)/packages-file-list$(3).txt; \
 	mv $(BUILD_DIR)/.files-list$(3).new \
-		$(BUILD_DIR)/.files-list$(3).stat
+		$(BUILD_DIR)/.files-list$(3).stat; \
+	exec 3>&-
 endef
 
 define step_pkg_size