From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kyukhin@tarantool.org>
Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id C8C2E4696C3
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 15 Apr 2020 18:45:28 +0300 (MSK)
Date: Wed, 15 Apr 2020 18:45:28 +0300
From: Kirill Yukhin <kyukhin@tarantool.org>
Message-ID: <20200415154528.llh2cghfba2faypw@tarantool.org>
References: <57e4a60ed158cec91fa18ed020dd0896d2da00f7.1576824858.git.avtikhon@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <57e4a60ed158cec91fa18ed020dd0896d2da00f7.1576824858.git.avtikhon@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v1] test: add ARM at travis-ci
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: "Alexander V. Tikhonov" <avtikhon@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hello,

Few comments inlined.


On 20 дек 09:55, Alexander V. Tikhonov wrote:
> Added ARM architecture for testing release and
> debug Tarantool builds on Travis-ci.
> 
> Close #4270
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4270-travis-ci-arm
> Issue: https://github.com/tarantool/tarantool/issues/4270
> 
>  .gitlab-ci.yml       | 11 +++++++++++
>  .travis.mk           | 12 ++++++++++++
>  src/lib/core/fiber.c | 17 ++++++++++++++++-
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 3af5a3c8a..1603b65e3 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -90,6 +90,17 @@ release_asan_clang8:
>    script:
>      - ${GITLAB_MAKE} test_asan_debian_no_deps
>  
> +release_arm:
> +  stage: test
> +  tags:
> +    - test
> +  variables:
> +    DOCKER_IMAGE: "${CI_REGISTRY}/${CI_PROJECT_PATH}/testing/arm64v8-debian-buster:latest"
> +    DOCKER_OPTIONS: "--network=host"
> +    CMAKE_EXTRA_PARAMS: -DENABLE_BACKTRACE=OFF
> +  script:
> +    - ${GITLAB_MAKE} test_arm_debian_no_deps
> +
>  osx_13_release:
>    <<: *release_only_definition
>    <<: *vbox_definition
> diff --git a/.travis.mk b/.travis.mk
> index 42969ff56..1427d5b2f 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -24,6 +24,7 @@ asan: docker_test_asan_debian
>  docker_%:
>  	mkdir -p ~/.cache/ccache
>  	docker run \
> +		${DOCKER_OPTIONS} \
>  		--rm=true --tty=true \
>  		--volume "${PWD}:/tarantool" \
>  		--volume "${HOME}/.cache:/cache" \
> @@ -123,6 +124,17 @@ test_asan_debian_no_deps: build_asan_debian
>  
>  test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps
>  
> +#######
> +# ARM #
> +#######
> +
> +qemu_arm_debian:
> +	docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
> +
> +build_arm_debian: qemu_arm_debian docker_build_debian
> +
> +test_arm_debian_no_deps: qemu_arm_debian docker_test_debian_no_deps

I am not familar w/ it at all, but just curiois, how do you instruct QEMU
to run ARM arch in this code?

> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 00ae8cded..6ef3c22db 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -43,7 +43,22 @@
>  #include "trigger.h"
>  
>  #if ENABLE_FIBER_TOP
> +#if !defined(__arm__) && !defined(__aarch64__) && !defined(__powerpc64__) && !defined(__PPC64__) && \
> +    !defined(__powerpc__)

Why not just check if it is x86?

>  #include <x86intrin.h> /* __rdtscp() */
> +#elif defined(__aarch64__)
> +static __inline__ unsigned long long __rdtscp(uint32_t *cpu_id)
> +{
> +       // System timer of ARMv8 runs at a different frequency than the CPU's.
> +       // The frequency is fixed, typically in the range 1-50MHz.  It can be
> +       // read at CNTFRQ special register.  We assume the OS has set up
> +       // the virtual timer properly.
> +       if (!cpu_id) { cpu_id = 0; }

You setup pointer to NULL here, why?

> @@ -109,7 +124,7 @@ cpu_stat_reset(struct cpu_stat *stat)
>  static uint64_t
>  cpu_stat_on_csw(struct cpu_stat *stat)
>  {
> -	uint32_t cpu_id;
> +	uint32_t cpu_id = 0;

This change is useless if you have __rdtscp defined.
Moreover, I'd introduced new routine, like get_clock() which will be aliased
to __rdscp on x86 and defined in your way for ARM.