From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH] mk: parallelize make config
Date: Sun, 29 Jan 2017 16:29:35 +0100
Message-ID: <1800797.YCEvINBUOG@xps13>
References: <20170122015034.19824-1-ferruh.yigit@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org
To: Ferruh Yigit <ferruh.yigit@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43])
 by dpdk.org (Postfix) with ESMTP id 61F92BD2C
 for <dev@dpdk.org>; Sun, 29 Jan 2017 16:29:39 +0100 (CET)
Received: by mail-wm0-f43.google.com with SMTP id r126so181120322wmr.0
 for <dev@dpdk.org>; Sun, 29 Jan 2017 07:29:39 -0800 (PST)
In-Reply-To: <20170122015034.19824-1-ferruh.yigit@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

2017-01-22 01:50, Ferruh Yigit:
> make config dependency resolving was always running serial,
> parallelize it for better performance.

It could be interesting to explain why it was not parallelized,
and how you made it possible.

The test script should be updated as below:

--- a/devtools/test-build.sh
+++ b/devtools/test-build.sh
-               make T=$2 O=$1 config
+               make -j$J T=$2 O=$1 config


> --- a/mk/internal/rte.depdirs-post.mk
> +++ b/mk/internal/rte.depdirs-post.mk
> @@ -29,11 +29,12 @@
>  #   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>  #   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
> -.PHONY: depdirs
> -depdirs:
> -	@for d in $(DEPDIRS-y); do \
> -		$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $$d ; \
> -	done
> +.PHONY: depdirs $(DEPDIRS-y)
> +depdirs: $(DEPDIRS-y)
> +	@echo ""

Why this echo "" ?

> +
> +$(DEPDIRS-y):
> +	@$(RTE_SDK)/buildtools/depdirs-rule.sh $(S) $@


> --- a/mk/rte.sdkdepdirs.mk
> +++ b/mk/rte.sdkdepdirs.mk
> @@ -36,19 +36,22 @@ ifeq (,$(wildcard $(RTE_OUTPUT)/Makefile))
>    $(error "need a make config first")
>  endif
>  
> +DEPDIRS = $(addsuffix /.depdirs, $(addprefix $(BUILDDIR)/,$(ROOTDIRS-y)))

These DEPDIRS are files, although DEPDIRS in other contexts are directories.
I think it should be renamed. DEPDIR_FILES?

>  # use a "for" in a shell to process dependencies: we don't want this
>  # task to be run in parallel.

You forgot to remove this obsolete comment.

>  .PHONY: depdirs
>  depdirs: $(RTE_OUTPUT)/.depdirs
> -$(RTE_OUTPUT)/.depdirs: $(RTE_OUTPUT)/.config
> -	@rm -f $(RTE_OUTPUT)/.depdirs ; \
> -	for d in $(ROOTDIRS-y); do \
> -		if [ -f $(RTE_SRCDIR)/$$d/Makefile ]; then \
> -			[ -d $(BUILDDIR)/$$d ] || mkdir -p $(BUILDDIR)/$$d ; \
> -			$(MAKE) S=$$d -f $(RTE_SRCDIR)/$$d/Makefile depdirs \
> -				>> $(RTE_OUTPUT)/.depdirs ; \
> -		fi ; \
> -	done
> +$(RTE_OUTPUT)/.depdirs: $(DEPDIRS)
> +	@rm -f $@
> +	@for f in $(DEPDIRS); do cat $$f >> $@; done
> +	@sort -u -o $@ $@
> +
> +$(DEPDIRS): $(RTE_OUTPUT)/.config
> +	@f=$(lastword $(subst /, ,$(dir $@))); \

Could you use $(notdir $(@D)) ?

> +	[ -d $(BUILDDIR)/$$f ] || mkdir -p $(BUILDDIR)/$$f; \
> +	rm -f $@; \

Why this removal?

> +	$(MAKE) S=$$f -f $(RTE_SRCDIR)/$$f/Makefile depdirs >> $@

This part is a bit complicated.
Could it be simplified by better naming $f?


> --- a/mk/rte.subdir.mk
> +++ b/mk/rte.subdir.mk
> @@ -76,7 +76,7 @@ clean: _postclean
>  # include .depdirs and define rules to order priorities between build
>  # of directories.
>  #
> -include $(RTE_OUTPUT)/.depdirs
> +-include $(RTE_OUTPUT)/.depdirs
>  
>  define depdirs_rule
>  $(1): $(sort $(patsubst $(S)/%,%,$(LOCAL_DEPDIRS-$(S)/$(1))))
> @@ -84,16 +84,15 @@ endef
>  
>  $(foreach d,$(DIRS-y),$(eval $(call depdirs_rule,$(d))))
>  
> +DEPDIRS = $(wildcard $(addprefix $(S)/,$(DIRS-y)))
>  
>  # use a "for" in a shell to process dependencies: we don't want this
>  # task to be run in parallel.

You forgot to remove this obsolete comment.

> -.PHONY: depdirs
> -depdirs:
> -	@for d in $(DIRS-y); do \
> -		if [ -f $(SRCDIR)/$$d/Makefile ]; then \
> -			$(MAKE) S=$S/$$d -f $(SRCDIR)/$$d/Makefile depdirs ; \
> -		fi ; \
> -	done
> +.PHONY: depdirs $(DEPDIRS)
> +depdirs: $(DEPDIRS)
> +
> +$(DEPDIRS):
> +	@$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile depdirs