diff mbox

[U-Boot,1/2] MAKEALL: Fix kill_children

Message ID 1351648522-29229-1-git-send-email-joe.hershberger@ni.com
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Joe Hershberger Oct. 31, 2012, 1:55 a.m. UTC 
When building in parallel, make sure that we look up the children
based on the the actual process group id instead of just assuming
that the MAKEALL pid is the process group id.

Also ensure that logs from incomplete builds are deleted in the
process.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---
 MAKEALL | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Simon Glass Oct. 31, 2012, 8:21 p.m. UTC | #1 
On Tue, Oct 30, 2012 at 6:55 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
> When building in parallel, make sure that we look up the children
> based on the the actual process group id instead of just assuming
> that the MAKEALL pid is the process group id.
>
> Also ensure that logs from incomplete builds are deleted in the
> process.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>

> ---
>  MAKEALL | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/MAKEALL b/MAKEALL
> index 84a5c92..1f88dc5 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -610,6 +610,13 @@ list_target() {
>  donep="${LOG_DIR}/._done_"
>  skipp="${LOG_DIR}/._skip_"
>
> +build_target_killed() {
> +       echo "Aborted $target build."
> +       # Remove the logs for this board since it was aborted
> +       rm -f ${LOG_DIR}/$target.MAKELOG ${LOG_DIR}/$target.ERR
> +       exit
> +}
> +
>  build_target() {
>         target=$1
>         build_idx=$2
> @@ -622,6 +629,7 @@ build_target() {
>         if [ $BUILD_MANY == 1 ] ; then
>                 output_dir="${OUTPUT_PREFIX}/${target}"
>                 mkdir -p "${output_dir}"
> +               trap build_target_killed TERM
>         else
>                 output_dir="${OUTPUT_PREFIX}"
>         fi
> @@ -640,6 +648,8 @@ build_target() {
>         fi
>
>         if [ $BUILD_MANY == 1 ] ; then
> +               trap - TERM
> +
>                 ${MAKE} -s tidy
>
>                 if [ -s ${LOG_DIR}/${target}.ERR ] ; then
> @@ -721,7 +731,9 @@ build_targets() {
>                         if [ $BUILD_MANY == 1 ] ; then
>                                 build_target ${t} ${TOTAL_CNT} &
>                         else
> +                               CUR_TGT="${t}"
>                                 build_target ${t} ${TOTAL_CNT}
> +                               CUR_TGT=''
>                         fi
>                 fi
>
> @@ -745,7 +757,11 @@ build_targets() {
>  #-----------------------------------------------------------------------
>
>  kill_children() {
> -       kill -- "-$1"
> +       local pgid=`ps -p $$ --no-headers -o "%r" | tr -d ' '`
> +       local children=`pgrep -g $pgid | grep -v $$ | grep -v $pgid`
> +
> +       kill $children 2> /dev/null
> +       wait $children 2> /dev/null
>
>         exit
>  }
> @@ -753,6 +769,9 @@ kill_children() {
>  print_stats() {
>         if [ "$ONLY_LIST" == 'y' ] ; then return ; fi
>
> +       # Only count boards that completed
> +       : $((TOTAL_CNT = `find ${skipp}* 2> /dev/null | wc -l`))
> +
>         rm -f ${donep}* ${skipp}*
>
>         if [ $BUILD_MANY == 1 ] && [ -e "${OUTPUT_PREFIX}/ERR" ] ; then
> @@ -762,6 +781,9 @@ print_stats() {
>                 WRN_LIST=`grep -riwL error ${OUTPUT_PREFIX}/ERR/`
>                 WRN_LIST=`for f in $WRN_LIST ; do echo -n " $(basename $f)" ; done`
>                 WRN_CNT=`echo $WRN_LIST | wc -w | awk '{print $1}'`
> +       else
> +               # Remove the logs for any board that was interrupted
> +               rm -f ${LOG_DIR}/${CUR_TGT}.MAKELOG ${LOG_DIR}/${CUR_TGT}.ERR
>         fi
>
>         echo ""
> @@ -776,7 +798,7 @@ print_stats() {
>         echo "----------------------------------------------------------"
>
>         if [ $BUILD_MANY == 1 ] ; then
> -               kill_children $$ &
> +               kill_children
>         fi
>
>         exit $RC
> --
> 1.7.11.5
>
Marek Vasut Nov. 1, 2012, 2:32 a.m. UTC | #2 
Dear Joe Hershberger,

> When building in parallel, make sure that we look up the children
> based on the the actual process group id instead of just assuming
> that the MAKEALL pid is the process group id.
> 
> Also ensure that logs from incomplete builds are deleted in the
> process.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
[...]

Nice $subject.

btw. is it possible to improve the u-boot build process parallelization?

Best regards,
Marek Vasut
Tom Rini Nov. 1, 2012, 3:02 a.m. UTC | #3 
On Thu, Nov 01, 2012 at 03:32:30AM +0100, Marek Vasut wrote:
> Dear Joe Hershberger,
> 
> > When building in parallel, make sure that we look up the children
> > based on the the actual process group id instead of just assuming
> > that the MAKEALL pid is the process group id.
> > 
> > Also ensure that logs from incomplete builds are deleted in the
> > process.
> > 
> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> [...]
> 
> Nice $subject.
> 
> btw. is it possible to improve the u-boot build process parallelization?

Probably a little, but not a lot?  It's be interesting to profile and
see where a single machine build has bottlenecks, if any.
Andy Fleming Nov. 1, 2012, 4:14 a.m. UTC | #4 
On Wed, Oct 31, 2012 at 9:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Dear Joe Hershberger,
>
>> When building in parallel, make sure that we look up the children
>> based on the the actual process group id instead of just assuming
>> that the MAKEALL pid is the process group id.
>>
>> Also ensure that logs from incomplete builds are deleted in the
>> process.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> [...]
>
> Nice $subject.
>
> btw. is it possible to improve the u-boot build process parallelization?

In what way?
Marek Vasut Nov. 1, 2012, 2:35 p.m. UTC | #5 
Dear Andy Fleming,

> On Wed, Oct 31, 2012 at 9:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Dear Joe Hershberger,
> > 
> >> When building in parallel, make sure that we look up the children
> >> based on the the actual process group id instead of just assuming
> >> that the MAKEALL pid is the process group id.
> >> 
> >> Also ensure that logs from incomplete builds are deleted in the
> >> process.
> >> 
> >> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > 
> > [...]
> > 
> > Nice $subject.
> > 
> > btw. is it possible to improve the u-boot build process parallelization?
> 
> In what way?

I recall someone complained the build process wasn't parallelizable enough and 
thus we need this stuff to run multiple builds at once in MAKEALL.

Best regards,
Marek Vasut
Tom Rini Nov. 1, 2012, 3:23 p.m. UTC | #6 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/01/12 07:35, Marek Vasut wrote:
> Dear Andy Fleming,
> 
>> On Wed, Oct 31, 2012 at 9:32 PM, Marek Vasut 
>> <marek.vasut@gmail.com> wrote:
[snip]
>>> btw. is it possible to improve the u-boot build process 
>>> parallelization?
>> 
>> In what way?
> 
> I recall someone complained the build process wasn't
> parallelizable enough and thus we need this stuff to run multiple
> builds at once in MAKEALL.

I don't know if the first half of that statement is strictly true.
It'd be interesting to profile a build on one of the very large boxes
but I think the issue is we only have ~300 objects being built.  It's
hard to run up a load of 32 when you only have that many things to
build.  But multiplying that by the number of boards in a arch or some
cpu or soc combinations is easier to run up the load, except around
the tails.  But, profiling, please!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQkpP3AAoJENk4IS6UOR1WkQgP/0oih/weogNgs7Ql9Iv4Ct06
pNlaYJiDEOlKxhdwG80zXnyWSqIKkLIYN09N5NPyYQ3qgeM+uRYRiReDiwudYY+P
pkOBWudj4X8SWUOucbRVWgx89DU4VrxCzY7sqqhDsQ+5ZTz8UjwG4wZaZOdewdLr
ktxe90NQGVsl6ozQtJoSoJ57y727e07piGuBISRDwYZPwmpCl8d9dNd6liRf7zdv
EXLIbO9aLROD67pkpO9OY392CVAU7cO4p5ir0dTGVZfYdPpadM+/Jsay5KsPhCf0
rM4eAFG1BvqhMBCc2NyPH2P71YU6rEsPdH6DyNh10aDRwUd7eep6/pU0tKbYpUR4
z0nsE1QOVLD5DY43/N0fEMN2G62LMiwnPuEQWT89N9uyDaFP7SS8lLRp/Sj7EhQF
NLEMtMKvN8bVIBiVFXW8WOVRJqlc5uVpoeRqfbOctl50x9zGNHhypJlH65qtEeg/
UlroM1tw3p0j6Xk9zrNioGFy5/mpkF1hw2pirUAKF06yomg0FM9oAOwp/tfjyjgU
uU2myW+lgWk0VfW/H00fyEBBs3vPnLo1PSL2WUKg43WRiEdY5C0/+E+lSIN7ph/z
eeWKelSu6BUWPkuqBnxhUkMFRYtPCNqE5x6BdVv5p8TPxacD842qDP930Vg3YhqQ
1KG0rtSB69QqzAtVYDmO
=4XRP
-----END PGP SIGNATURE-----
Tom Rini Nov. 3, 2012, 10:54 p.m. UTC | #7 
On Tue, Oct 30, 2012 at 08:55:20PM -0500, Joe Hershberger wrote:

> When building in parallel, make sure that we look up the children
> based on the the actual process group id instead of just assuming
> that the MAKEALL pid is the process group id.
> 
> Also ensure that logs from incomplete builds are deleted in the
> process.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Now I see:
/home/trini/bin/uboot-build.sh: line 110:  2024 Terminated
/usr/bin/time -o $TIMEFILE -f %e ./MAKEALL $SOC $MACHINE > $LOG 2>&1

With my MAKEALL wrapper.  Anything we can do about that?
Joe Hershberger Nov. 5, 2012, 12:13 a.m. UTC | #8 
Hi Tom,

On Sat, Nov 3, 2012 at 5:54 PM, Tom Rini <trini@ti.com> wrote:
> On Tue, Oct 30, 2012 at 08:55:20PM -0500, Joe Hershberger wrote:
>
>> When building in parallel, make sure that we look up the children
>> based on the the actual process group id instead of just assuming
>> that the MAKEALL pid is the process group id.
>>
>> Also ensure that logs from incomplete builds are deleted in the
>> process.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> Now I see:
> /home/trini/bin/uboot-build.sh: line 110:  2024 Terminated
> /usr/bin/time -o $TIMEFILE -f %e ./MAKEALL $SOC $MACHINE > $LOG 2>&1
>
> With my MAKEALL wrapper.  Anything we can do about that?

I see that as well... I did a little research and it seems like you
have to jump through some pretty serious hoops to get rid of that
message.  I don't think it's worth the effort.

Does it have an impact on something in your wrapper?

-Joe
Tom Rini Nov. 5, 2012, 12:53 a.m. UTC | #9 
On Sun, Nov 04, 2012 at 06:13:03PM -0600, Joe Hershberger wrote:
> Hi Tom,
> 
> On Sat, Nov 3, 2012 at 5:54 PM, Tom Rini <trini@ti.com> wrote:
> > On Tue, Oct 30, 2012 at 08:55:20PM -0500, Joe Hershberger wrote:
> >
> >> When building in parallel, make sure that we look up the children
> >> based on the the actual process group id instead of just assuming
> >> that the MAKEALL pid is the process group id.
> >>
> >> Also ensure that logs from incomplete builds are deleted in the
> >> process.
> >>
> >> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> >
> > Now I see:
> > /home/trini/bin/uboot-build.sh: line 110:  2024 Terminated
> > /usr/bin/time -o $TIMEFILE -f %e ./MAKEALL $SOC $MACHINE > $LOG 2>&1
> >
> > With my MAKEALL wrapper.  Anything we can do about that?
> 
> I see that as well... I did a little research and it seems like you
> have to jump through some pretty serious hoops to get rid of that
> message.  I don't think it's worth the effort.
> 
> Does it have an impact on something in your wrapper?

Purely cosemetic so I can live with the bugfixes as-is, thanks!
Tom Rini Dec. 7, 2012, 3:50 p.m. UTC | #10 
On Tue, Oct 30, 2012 at 08:55:20PM -0500, Joe Hershberger wrote:

> When building in parallel, make sure that we look up the children
> based on the the actual process group id instead of just assuming
> that the MAKEALL pid is the process group id.
> 
> Also ensure that logs from incomplete builds are deleted in the
> process.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Applied to u-boot/master, thanks!
diff mbox

Patch

diff --git a/MAKEALL b/MAKEALL
index 84a5c92..1f88dc5 100755
--- a/MAKEALL
+++ b/MAKEALL
@@ -610,6 +610,13 @@  list_target() {
 donep="${LOG_DIR}/._done_"
 skipp="${LOG_DIR}/._skip_"
 
+build_target_killed() {
+	echo "Aborted $target build."
+	# Remove the logs for this board since it was aborted
+	rm -f ${LOG_DIR}/$target.MAKELOG ${LOG_DIR}/$target.ERR
+	exit
+}
+
 build_target() {
 	target=$1
 	build_idx=$2
@@ -622,6 +629,7 @@  build_target() {
 	if [ $BUILD_MANY == 1 ] ; then
 		output_dir="${OUTPUT_PREFIX}/${target}"
 		mkdir -p "${output_dir}"
+		trap build_target_killed TERM
 	else
 		output_dir="${OUTPUT_PREFIX}"
 	fi
@@ -640,6 +648,8 @@  build_target() {
 	fi
 
 	if [ $BUILD_MANY == 1 ] ; then
+		trap - TERM
+
 		${MAKE} -s tidy
 
 		if [ -s ${LOG_DIR}/${target}.ERR ] ; then
@@ -721,7 +731,9 @@  build_targets() {
 			if [ $BUILD_MANY == 1 ] ; then
 				build_target ${t} ${TOTAL_CNT} &
 			else
+				CUR_TGT="${t}"
 				build_target ${t} ${TOTAL_CNT}
+				CUR_TGT=''
 			fi
 		fi
 
@@ -745,7 +757,11 @@  build_targets() {
 #-----------------------------------------------------------------------
 
 kill_children() {
-	kill -- "-$1"
+	local pgid=`ps -p $$ --no-headers -o "%r" | tr -d ' '`
+	local children=`pgrep -g $pgid | grep -v $$ | grep -v $pgid`
+
+	kill $children 2> /dev/null
+	wait $children 2> /dev/null
 
 	exit
 }
@@ -753,6 +769,9 @@  kill_children() {
 print_stats() {
 	if [ "$ONLY_LIST" == 'y' ] ; then return ; fi
 
+	# Only count boards that completed
+	: $((TOTAL_CNT = `find ${skipp}* 2> /dev/null | wc -l`))
+
 	rm -f ${donep}* ${skipp}*
 
 	if [ $BUILD_MANY == 1 ] && [ -e "${OUTPUT_PREFIX}/ERR" ] ; then
@@ -762,6 +781,9 @@  print_stats() {
 		WRN_LIST=`grep -riwL error ${OUTPUT_PREFIX}/ERR/`
 		WRN_LIST=`for f in $WRN_LIST ; do echo -n " $(basename $f)" ; done`
 		WRN_CNT=`echo $WRN_LIST | wc -w | awk '{print $1}'`
+	else
+		# Remove the logs for any board that was interrupted
+		rm -f ${LOG_DIR}/${CUR_TGT}.MAKELOG ${LOG_DIR}/${CUR_TGT}.ERR
 	fi
 
 	echo ""
@@ -776,7 +798,7 @@  print_stats() {
 	echo "----------------------------------------------------------"
 
 	if [ $BUILD_MANY == 1 ] ; then
-		kill_children $$ &
+		kill_children
 	fi
 
 	exit $RC