From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755619AbbIHOeT (ORCPT ); Tue, 8 Sep 2015 10:34:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34363 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755586AbbIHOeK (ORCPT ); Tue, 8 Sep 2015 10:34:10 -0400 Date: Tue, 8 Sep 2015 16:34:07 +0200 From: Andrea Arcangeli To: Bamvor Zhang Jian Cc: Michael Ellerman , linux-kernel@vger.kernel.org, Mark Brown , khilman@linaro.org, tyler.baker@linaro.org, shuahkh@osg.samsung.com Subject: Re: [PATCH 6/7] selftests: only compile userfaultfd for x86 and powperpc Message-ID: <20150908143407.GA10639@redhat.com> 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> <55EED3C9.2090007@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55EED3C9.2090007@linaro.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 08, 2015 at 08:25:45PM +0800, Bamvor Zhang Jian wrote: > 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. I already had a few minor changes queued to be submitted for arm and ppc and a few updates to the selftest. I didn't like that you had to remember running make headers_install for changes like the below one to build, so I added the dependency so that "make" still works without having to run other commands before it. These aren't reviewed yet. https://git.kernel.org/cgit/linux/kernel/git/andrea/aa.git/commit/?id=5ce2efeb91b501aa1bc7370f43732681fa9123e2 I was planning to send these non-x86 updates to Andrew for review and merging... Isn't this necessary as well? https://git.kernel.org/cgit/linux/kernel/git/andrea/aa.git/commit/?id=0eb943b76537a93fc4dd85cc0cbf937ce8266228 I can include the below one too, but we need to coordinate to submit them or eventually some will reject. > > 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 > > > > >