[v2,1/2] selftests: proc: Fix proc-empty-vm build error on non x86_64

Message ID 20221109221104.1797802-1-punit.agrawal@bytedance.com
State New
Headers
Series [v2,1/2] selftests: proc: Fix proc-empty-vm build error on non x86_64 |

Commit Message

Punit Agrawal Nov. 9, 2022, 10:11 p.m. UTC
  The proc-empty-vm test is implemented for x86_64 and fails to build
for other architectures. Rather then emitting a compiler error it
would be preferable to only build the test on supported architectures.

Mark proc-empty-vm as a test for x86_64 and customise the Makefile to
build it only when building for this target architecture.

Fixes: 5bc73bb3451b ("proc: test how it holds up with mapping'less process")
Signed-off-by: Punit Agrawal <punit.agrawal@bytedance.com>
---
v1 -> v2
* Fixed missing compilation on x86_64

Previous version
* https://lore.kernel.org/all/20221109110621.1791999-1-punit.agrawal@bytedance.com/

tools/testing/selftests/proc/Makefile | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
  

Comments

Andrew Morton Nov. 10, 2022, 12:02 a.m. UTC | #1
On Wed,  9 Nov 2022 22:11:03 +0000 Punit Agrawal <punit.agrawal@bytedance.com> wrote:

> The proc-empty-vm test is implemented for x86_64 and fails to build
> for other architectures. Rather then emitting a compiler error it
> would be preferable to only build the test on supported architectures.

Why does it fail?  What would be involved in making it available
on other architectures?
  
Shuah Khan Nov. 10, 2022, 12:20 a.m. UTC | #2
On 11/9/22 15:11, Punit Agrawal wrote:
> The proc-empty-vm test is implemented for x86_64 and fails to build
> for other architectures. Rather then emitting a compiler error it
> would be preferable to only build the test on supported architectures.
> 
> Mark proc-empty-vm as a test for x86_64 and customise the Makefile to
> build it only when building for this target architecture.
> 
> Fixes: 5bc73bb3451b ("proc: test how it holds up with mapping'less process")
> Signed-off-by: Punit Agrawal <punit.agrawal@bytedance.com>
> ---
> v1 -> v2
> * Fixed missing compilation on x86_64
> 
> Previous version
> * https://lore.kernel.org/all/20221109110621.1791999-1-punit.agrawal@bytedance.com/
> 
> tools/testing/selftests/proc/Makefile | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/proc/Makefile b/tools/testing/selftests/proc/Makefile
> index cd95369254c0..743aaa0cdd52 100644
> --- a/tools/testing/selftests/proc/Makefile
> +++ b/tools/testing/selftests/proc/Makefile
> @@ -1,14 +1,16 @@
>   # SPDX-License-Identifier: GPL-2.0-only
> +
> +# When ARCH not overridden for crosscompiling, lookup machine
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
>   CFLAGS += -Wall -O2 -Wno-unused-function
>   CFLAGS += -D_GNU_SOURCE
>   LDFLAGS += -pthread
>   
> -TEST_GEN_PROGS :=
>   TEST_GEN_PROGS += fd-001-lookup
>   TEST_GEN_PROGS += fd-002-posix-eq
>   TEST_GEN_PROGS += fd-003-kthread
>   TEST_GEN_PROGS += proc-loadavg-001
> -TEST_GEN_PROGS += proc-empty-vm
>   TEST_GEN_PROGS += proc-pid-vm
>   TEST_GEN_PROGS += proc-self-map-files-001
>   TEST_GEN_PROGS += proc-self-map-files-002
> @@ -26,4 +28,8 @@ TEST_GEN_PROGS += thread-self
>   TEST_GEN_PROGS += proc-multiple-procfs
>   TEST_GEN_PROGS += proc-fsconfig-hidepid
>   
> +TEST_GEN_PROGS_x86_64 += proc-empty-vm

Why do you need this? You already have conditional compiles.
Conditionally add proc-empty-vm to TEST_GEN_PROGS like other
tests do.

> +
> +TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH))
> +
>   include ../lib.mk

Same question Andrews asked you. What does it take to get this
to work on other architectures. proc and vm tests should be
arch. agnostic as a rule unless it is absolutely necessary to
have them acrh. aware.

thanks,
-- Shuah

thanks,
-- Shuah
  
Shuah Khan Nov. 10, 2022, 1:44 a.m. UTC | #3
On 11/9/22 17:02, Andrew Morton wrote:
> On Wed,  9 Nov 2022 22:11:03 +0000 Punit Agrawal <punit.agrawal@bytedance.com> wrote:
> 
>> The proc-empty-vm test is implemented for x86_64 and fails to build
>> for other architectures. Rather then emitting a compiler error it
>> would be preferable to only build the test on supported architectures.
> 
> Why does it fail?  What would be involved in making it available
> on other architectures?
> 

I have the same question and also don't like adding TEST_GEN_PROGS_x86_64
Please see my comments on the two patches regarding this.

thanks,
-- Shuah
  
Punit Agrawal Nov. 10, 2022, 2:34 p.m. UTC | #4
Hi Andrew,

Thanks for taking a look.

Andrew Morton <akpm@linux-foundation.org> writes:

> On Wed,  9 Nov 2022 22:11:03 +0000 Punit Agrawal <punit.agrawal@bytedance.com> wrote:
>
>> The proc-empty-vm test is implemented for x86_64 and fails to build
>> for other architectures. Rather then emitting a compiler error it
>> would be preferable to only build the test on supported architectures.
>
> Why does it fail?  What would be involved in making it available
> on other architectures?

The test is written to fail building on architectures other than
x86_64.

    #ifdef __amd64__
                    munmap(NULL, ((size_t)1 << 47) - 4096);
    #else
    #error "implement 'unmap everything'"
    #endif

I hit the build failure while semi-automating the running of tests on
internal infrastructure. 

I am not familiar with the issue being tested but after a bit of
staring, it looks like there are two architecture dependent components
to the tests -

    1. TASK_SIZE / application memory layout - the test unmaps the
       entire the user virtual address space. For this, it needs to know
       the length to pass to munmap().

       Although it's possible to add this per-architecture, I am not
       sure if there is a way to discover the length passed to munmap().

    2. How the vsyscall page (if implemented) is mapped - this
       influences the known good values used for comparison in the test.

       It doesn't look like vsyscall page is used on arm64 but I am not
       sure about the situation with other architectures.

(Alexey, please add if I've missed anything)

Thanks,
Punit
  
Punit Agrawal Nov. 10, 2022, 2:45 p.m. UTC | #5
Hi Shuah,

Shuah Khan <skhan@linuxfoundation.org> writes:

> On 11/9/22 15:11, Punit Agrawal wrote:
>> The proc-empty-vm test is implemented for x86_64 and fails to build
>> for other architectures. Rather then emitting a compiler error it
>> would be preferable to only build the test on supported architectures.
>> Mark proc-empty-vm as a test for x86_64 and customise the Makefile
>> to
>> build it only when building for this target architecture.
>> Fixes: 5bc73bb3451b ("proc: test how it holds up with mapping'less
>> process")
>> Signed-off-by: Punit Agrawal <punit.agrawal@bytedance.com>
>> ---
>> v1 -> v2
>> * Fixed missing compilation on x86_64
>> Previous version
>> * https://lore.kernel.org/all/20221109110621.1791999-1-punit.agrawal@bytedance.com/
>> tools/testing/selftests/proc/Makefile | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>> diff --git a/tools/testing/selftests/proc/Makefile
>> b/tools/testing/selftests/proc/Makefile
>> index cd95369254c0..743aaa0cdd52 100644
>> --- a/tools/testing/selftests/proc/Makefile
>> +++ b/tools/testing/selftests/proc/Makefile
>> @@ -1,14 +1,16 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>> +
>> +# When ARCH not overridden for crosscompiling, lookup machine
>> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>> +
>>   CFLAGS += -Wall -O2 -Wno-unused-function
>>   CFLAGS += -D_GNU_SOURCE
>>   LDFLAGS += -pthread
>>   -TEST_GEN_PROGS :=
>>   TEST_GEN_PROGS += fd-001-lookup
>>   TEST_GEN_PROGS += fd-002-posix-eq
>>   TEST_GEN_PROGS += fd-003-kthread
>>   TEST_GEN_PROGS += proc-loadavg-001
>> -TEST_GEN_PROGS += proc-empty-vm
>>   TEST_GEN_PROGS += proc-pid-vm
>>   TEST_GEN_PROGS += proc-self-map-files-001
>>   TEST_GEN_PROGS += proc-self-map-files-002
>> @@ -26,4 +28,8 @@ TEST_GEN_PROGS += thread-self
>>   TEST_GEN_PROGS += proc-multiple-procfs
>>   TEST_GEN_PROGS += proc-fsconfig-hidepid
>>   +TEST_GEN_PROGS_x86_64 += proc-empty-vm
>
> Why do you need this? You already have conditional compiles.
> Conditionally add proc-empty-vm to TEST_GEN_PROGS like other
> tests do.

I copied this approach from KVM tests. Looks like we've got a few
different ways of disabling compilation within selftests.

I can respin to conditionally compile as suggested if that is the way
forward.

>> +
>> +TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH))
>> +
>>   include ../lib.mk
>
> Same question Andrews asked you. What does it take to get this
> to work on other architectures. proc and vm tests should be
> arch. agnostic as a rule unless it is absolutely necessary to
> have them acrh. aware.

Please see my reply elsewhere in the thread for an assessment of the
architecture dependencies.
  

Patch

diff --git a/tools/testing/selftests/proc/Makefile b/tools/testing/selftests/proc/Makefile
index cd95369254c0..743aaa0cdd52 100644
--- a/tools/testing/selftests/proc/Makefile
+++ b/tools/testing/selftests/proc/Makefile
@@ -1,14 +1,16 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
+
+# When ARCH not overridden for crosscompiling, lookup machine
+ARCH ?= $(shell uname -m 2>/dev/null || echo not)
+
 CFLAGS += -Wall -O2 -Wno-unused-function
 CFLAGS += -D_GNU_SOURCE
 LDFLAGS += -pthread
 
-TEST_GEN_PROGS :=
 TEST_GEN_PROGS += fd-001-lookup
 TEST_GEN_PROGS += fd-002-posix-eq
 TEST_GEN_PROGS += fd-003-kthread
 TEST_GEN_PROGS += proc-loadavg-001
-TEST_GEN_PROGS += proc-empty-vm
 TEST_GEN_PROGS += proc-pid-vm
 TEST_GEN_PROGS += proc-self-map-files-001
 TEST_GEN_PROGS += proc-self-map-files-002
@@ -26,4 +28,8 @@  TEST_GEN_PROGS += thread-self
 TEST_GEN_PROGS += proc-multiple-procfs
 TEST_GEN_PROGS += proc-fsconfig-hidepid
 
+TEST_GEN_PROGS_x86_64 += proc-empty-vm
+
+TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH))
+
 include ../lib.mk