From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754854AbbIHM25 (ORCPT ); Tue, 8 Sep 2015 08:28:57 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:34730 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754175AbbIHM2x (ORCPT ); Tue, 8 Sep 2015 08:28:53 -0400 Subject: Re: [PATCH 6/7] selftests: only compile userfaultfd for x86 and powperpc To: Michael Ellerman References: <1439559818-21666-1-git-send-email-bamvor.zhangjian@linaro.org> <1439559818-21666-7-git-send-email-bamvor.zhangjian@linaro.org> <1440991580.5735.4.camel@ellerman.id.au> <55EEA731.5090708@linaro.org> <1441706053.7601.9.camel@ellerman.id.au> Cc: linux-kernel@vger.kernel.org, Mark Brown , khilman@linaro.org, tyler.baker@linaro.org, shuahkh@osg.samsung.com, Andrea Arcangeli From: Bamvor Zhang Jian Message-ID: <55EED3C9.2090007@linaro.org> Date: Tue, 8 Sep 2015 20:25:45 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1441706053.7601.9.camel@ellerman.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Michael On 09/08/2015 05:54 PM, Michael Ellerman wrote: > On Tue, 2015-09-08 at 17:15 +0800, Bamvor Zhang Jian wrote: >> Hi, Michael >> >> I thought I reply to you, but ... >> >> On 08/31/2015 11:26 AM, Michael Ellerman wrote: >>> On Fri, 2015-08-14 at 21:43 +0800, Bamvor Jian Zhang wrote: >>>> Signed-off-by: Bamvor Jian Zhang >>>> --- >>>> tools/testing/selftests/vm/Makefile | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile >>>> index bb888c6..4dd6e4f 100644 >>>> --- a/tools/testing/selftests/vm/Makefile >>>> +++ b/tools/testing/selftests/vm/Makefile >>>> @@ -1,5 +1,15 @@ >>>> # Makefile for vm selftests >>>> >>>> +uname_M := $(shell uname -m 2>/dev/null || echo not) >>>> +ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/) >>>> + >>>> +ifeq ($(ARCH),powerpc) >>>> +support_userfaultfd = yes >>>> +endif >>>> +ifeq ($(ARCH),x86) >>>> +support_userfaultfd = yes >>>> +endif >>>> + >>>> CFLAGS = -Wall >>>> BINARIES = compaction_test >>>> BINARIES += hugepage-mmap >>>> @@ -9,7 +19,9 @@ BINARIES += mlock2-tests >>>> BINARIES += on-fault-limit >>>> BINARIES += thuge-gen >>>> BINARIES += transhuge-stress >>>> +ifdef support_userfaultfd >>>> BINARIES += userfaultfd >>>> +endif >>>> >>>> all: $(BINARIES) >>>> %: %.c >>> >>> >>> This is nasty. It means when userfaultfd gets implemented for other arches >>> someone has to remember to update the logic here, which they won't. >>> >>> Instead the C program should just do nothing when __NR_userfaultfd is not defined, eg: >>> >>> #ifdef __NR_userfaultfd >>> >>> int main(int argc, char **argv) >>> { >>> ... >>> } >>> >>> #else >>> >>> int main(void) >>> { >>> printf("skip: Skipping userfaultfd test\n"); >>> return 0; >>> } >>> #endif >>> >>> >>> This way when the syscall is implemented for other arches the test will just >>> start working. >>> >>> cheers >>> >>> >> When read the following code, It seems that sometimes __NR_userfaultfd is not >> defined but the syscall is exist. I am not sure why these piece is needed. >> cc'd c >> >> #ifndef __NR_userfaultfd >> #ifdef __x86_64__ >> #define __NR_userfaultfd 323 >> #elif defined(__i386__) >> #define __NR_userfaultfd 374 >> #elif defined(__powewrpc__) >> #define __NR_userfaultfd 364 >> #else >> #error "missing __NR_userfaultfd definition" >> #endif >> #endif >> >> Do you mean that we should remove the above code? > > Well yes, it would need to be removed to make the logic I suggested work. > > I'm not sure those #defines actually help in practice, because if the syscall > number is not defined then linux/userfaultfd.h will not exist and the whole > test will not compile anyway. > > I was suggesting something like this, which has the properties of: > - not breaking the build on arches that don't have the syscall > - still printing a notice on arches that don't have the syscall, both at build > time and runtime. > - building correctly on an arch as soon as that arch implements the syscall, > with no extra changes required. Ok, I agree with you. I will send the updated patch later. > cheers > > > diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c > index 2bf1fc3f562b..652c9d805006 100644 > --- a/tools/testing/selftests/vm/userfaultfd.c > +++ b/tools/testing/selftests/vm/userfaultfd.c > @@ -64,19 +64,10 @@ > #include > #include > #include > -#include > > -#ifndef __NR_userfaultfd > -#ifdef __x86_64__ > -#define __NR_userfaultfd 323 > -#elif defined(__i386__) > -#define __NR_userfaultfd 374 > -#elif defined(__powewrpc__) > -#define __NR_userfaultfd 364 > -#else > -#error "missing __NR_userfaultfd definition" > -#endif > -#endif > +#ifdef __NR_userfaultfd > + > +#include > > static unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; > > @@ -636,3 +627,15 @@ int main(int argc, char **argv) > nr_pages, nr_pages_per_cpu); > return userfaultfd_stress(); > } > + > +#else /* ! __NR_userfaultfd */ > + > +#warning "missing __NR_userfaultfd definition" > + > +int main(void) > +{ > + printf("skip: Skipping userfaultfd test (missing __NR_userfaultfd)\n"); > + return 0; > +} > + > +#endif > >