Re: [PATCH 2/3] selftests: add membarrier syscall test

[Mathieu Desnoyers <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>]

----- On Sep 8, 2015, at 12:19 AM, Michael Ellerman mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org wrote:

> On Mon, 2015-09-07 at 16:01 +0000, Mathieu Desnoyers wrote:
>> ----- On Sep 3, 2015, at 11:36 PM, Michael Ellerman mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org wrote:
>> > On Thu, 2015-09-03 at 15:47 +0000, Mathieu Desnoyers wrote:
>> >> 
>> >> My personal experience is that make headers_install does not necessarily play
>> >> well with the distribution header file hierarchy, which requires some tweaks
>> >> to be done by the users (e.g. asm vs x86_64-linux-gnu).
>> > 
>> > OK, I've never had issues. What exactly are you doing and how is it going wrong?
>> 
>> After some investigation, I noticed the following:
>> 
>> 1) I first ran make headers_install as root, which installed the
>> headers within my build tree. I later tried it again as user, and
>> it failed due to permission issues (my bad). This is where I tried
>> to install it into my system rather than under my build directory,
>> which caused a mess.
> 
> Yeah OK that's a good point about root.
> 
> I tend to build as a regular user and then copy the installed tests to another
> machine where I run them as root.
> 
>> 2) Since make kselftest should be run as root (according to make
>> help),
> 
> Well some of the tests only work when run as root. IMHO we should support
> running as many tests as possible as non-root, but some of them obviously
> require root.
> 
> So you can run them as non-root, but to get maximum coverage you need to run
> them as root.

Works for me. We do something similar in lttng-tools. We use "tap"
(https://testanything.org/) for tests, and explicitly skip all tests that
require root if we detect that we don't run as root. I notice that many
selftests format their own output. The nice part about standardizing on
something like tap is that it simplifies automated parsing of the test
output.

> 
>> this means that all the output files generated by the build
>> are owned by root. It leads to permissions issues when trying to
>> rebuild the tests as user afterward. Perhaps we could introduce a
>> distinction between make kselftest_build and make kselftest_run ?
>> The former could be executed as user, and the latter as root.
> 
> Right. Personally I don't use the kselftest target at all, I just cd down to
> tools/testing/selftests and run make there.
> 
> If it was up to me the kselftest target would go away, because it's only caused
> us trouble so far.
> 
> But given it's there we should try to make it work as well as possible. So yeah
> splitting it into build and run would make sense, that way you could do:
> 
> $ make headers_install
> $ make kselftest_build
> $ sudo make kselftest_run
> 
> And that would hopefully do the right thing.
> 
> Would that improve the workflow for you?

Yes. Although I'm wondering why the kernel should be different from many
other projects out there. Why not simply:

- Add a kselftest_build dependency to the kernel build, so tests are always built,
  and warnings that arise from modifying anything related to installed headers
  will trigger for everyone,
- Add a dependency on headers_install into the obj tree to kselftest_build,
- Optionally add a "make check" alias to "make kselftest".

This way, running the tests becomes as simple as:

make
sudo make check

Documentation is key here: make sure to update Documentation/kselftest.txt to
document where the self-tests are looking for their system headers (not system,
but within usr/ in the obj tree). This is the missing documentation bit that
confused me the most.

> 
>> > So that seems to be working for me. Are you doing some different work flow, or
>> > am I just missing something?
>> 
>> When doing make headers_install, it indeed installs
>> membarrier.h where we expect it under the build output
>> dir:
>> 
>> $ ls usr/include/linux/membarrier.h
>> usr/include/linux/membarrier.h
>> 
>> However, if I issue
>> 
>> $ make -C tools/testing/selftests TARGETS=membarrier
>> make: Entering directory `/home/efficios/git/linux-next/tools/testing/selftests'
>> for TARGET in membarrier; do \
>> 		make -C $TARGET; \
>> 	done;
>> make[1]: Entering directory
>> `/home/efficios/git/linux-next/tools/testing/selftests/membarrier'
>> gcc     membarrier_test.c   -o membarrier_test
>> membarrier_test.c:2:30: fatal error: linux/membarrier.h: No such file or
>> directory
>>  #include <linux/membarrier.h>
>> 
>> This is after applying the modifications you requested
>> (see patch attached). Perhaps I did something wrong ?
> 
> Yeah sorry, you still need the -I line:
> 
> CFLAGS += -I../../../../usr/include/
> 
> 
> We /should/ add that to lib.mk so it's inherited by everyone, but we haven't
> yet.

Yep, this would be a good start.

> 
> So I think if you put that back the instructions I gave you will work?

Yes, it does, thanks!

Mathieu

> 
> cheers

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com