Re: [dpdk-dev] [PATCH] mk: parallelize make config

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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