Re: [PATCHv5 1/1] [tools/selftests]: android/ion: userspace test utility for ion buffer sharing

[Pintu Kumar]

On Thu, Nov 2, 2017 at 8:37 PM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
> Hi Pintu,
>
> On 11/01/2017 11:00 AM, Pintu Agarwal wrote:
>> This is a test utility to verify ION buffer sharing in user space
>> between 2 independent processes.
>> It uses unix domain socket (with SCM_RIGHTS) as IPC to transfer an FD to
>> another process to share the same buffer.
>> This utility demonstrates how ION buffer sharing can be implemented between
>> two user space processes, using various heap types.
>>
>> This utility is made to be run as part of kselftest framework in kernel.
>> The utility is verified on Ubuntu-32 bit system with Linux Kernel 4.14,
>> using ION system heap.
>>
>> For more information about the utility please check the README file.
>>
>> Signed-off-by: Pintu Agarwal <pintu.ping@xxxxxxxxx>
>> ---
>>  tools/testing/selftests/Makefile                   |   3 +-
>>  tools/testing/selftests/android/Makefile           |  46 ++++
>>  tools/testing/selftests/android/ion/.gitignore     |   2 +
>>  tools/testing/selftests/android/ion/Makefile       |  16 ++
>>  tools/testing/selftests/android/ion/README         | 101 ++++++++
>>  tools/testing/selftests/android/ion/config         |   4 +
>>  tools/testing/selftests/android/ion/ion.h          | 143 ++++++++++++
>>  tools/testing/selftests/android/ion/ion_test.sh    |  55 +++++
>>  .../testing/selftests/android/ion/ionapp_export.c  | 135 +++++++++++
>>  .../testing/selftests/android/ion/ionapp_import.c  |  88 +++++++
>>  tools/testing/selftests/android/ion/ionutils.c     | 259 +++++++++++++++++++++
>>  tools/testing/selftests/android/ion/ionutils.h     |  55 +++++
>>  tools/testing/selftests/android/ion/ipcsocket.c    | 227 ++++++++++++++++++
>>  tools/testing/selftests/android/ion/ipcsocket.h    |  35 +++
>>  tools/testing/selftests/android/run.sh             |   3 +
>>  15 files changed, 1171 insertions(+), 1 deletion(-)
>>  create mode 100644 tools/testing/selftests/android/Makefile
>>  create mode 100644 tools/testing/selftests/android/ion/.gitignore
>>  create mode 100644 tools/testing/selftests/android/ion/Makefile
>>  create mode 100644 tools/testing/selftests/android/ion/README
>>  create mode 100644 tools/testing/selftests/android/ion/config
>>  create mode 100644 tools/testing/selftests/android/ion/ion.h
>>  create mode 100755 tools/testing/selftests/android/ion/ion_test.sh
>>  create mode 100644 tools/testing/selftests/android/ion/ionapp_export.c
>>  create mode 100644 tools/testing/selftests/android/ion/ionapp_import.c
>>  create mode 100644 tools/testing/selftests/android/ion/ionutils.c
>>  create mode 100644 tools/testing/selftests/android/ion/ionutils.h
>>  create mode 100644 tools/testing/selftests/android/ion/ipcsocket.c
>>  create mode 100644 tools/testing/selftests/android/ion/ipcsocket.h
>>  create mode 100755 tools/testing/selftests/android/run.sh
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index ff80564..61bc77b 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -1,4 +1,5 @@
>> -TARGETS =  bpf
>> +TARGETS = android
>
> Based on our discussion on dependency on staging headers, let's not add
> this test to kselftest run. Remove it from here and users can run it
> as needed.
>
>> +TARGETS += bpf
>>  TARGETS += breakpoints
>>  TARGETS += capabilities
>>  TARGETS += cpufreq
>> diff --git a/tools/testing/selftests/android/Makefile b/tools/testing/selftests/android/Makefil> new file mode 100644
>> index 0000000..1a74922
>> --- /dev/null
>> +++ b/tools/testing/selftests/android/Makefile
>> @@ -0,0 +1,46 @@
>> +SUBDIRS := ion
>> +
>> +TEST_PROGS := run.sh
>> +
>> +.PHONY: all clean
>> +
>> +include ../lib.mk
>> +
>> +all:
>> +     @for DIR in $(SUBDIRS); do              \
>> +             BUILD_TARGET=$(OUTPUT)/$$DIR;   \
>> +             mkdir $$BUILD_TARGET  -p;       \
>> +             make OUTPUT=$$BUILD_TARGET -C $$DIR $@;\
>> +             #SUBDIR test prog name should be in the form: SUBDIR_test.sh
>> +             TEST=$$DIR"_test.sh"; \
>> +             if [ -e $$DIR/$$TEST ]; then
>> +                     rsync -a $$DIR/$$TEST $$BUILD_TARGET/;
>> +             fi
>> +     done
>> +
>> +override define RUN_TESTS
>> +     @cd $(OUTPUT); ./run.sh
>> +endef
>> +
>> +override define INSTALL_RULE
>> +     mkdir -p $(INSTALL_PATH)
>> +     install -t $(INSTALL_PATH) $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)
>> +
>> +     @for SUBDIR in $(SUBDIRS); do \
>> +             BUILD_TARGET=$(OUTPUT)/$$SUBDIR;        \
>> +             mkdir $$BUILD_TARGET  -p;       \
>> +             $(MAKE) OUTPUT=$$BUILD_TARGET -C $$SUBDIR INSTALL_PATH=$(INSTALL_PATH)/$$SUBDIR install; \
>> +     done;
>> +endef
>> +
>> +override define EMIT_TESTS
>> +     echo "./run.sh"
>> +endef
>> +
>> +override define CLEAN
>> +     @for DIR in $(SUBDIRS); do              \
>> +             BUILD_TARGET=$(OUTPUT)/$$DIR;   \
>> +             mkdir $$BUILD_TARGET  -p;       \
>> +             make OUTPUT=$$BUILD_TARGET -C $$DIR $@;\
>> +     done
>> +endef
>
> This Makefile is fine, you still want to leverage common run_tests target
> in lib.mk Install and emit tests should be fine, because these are built
> in the source tree. I am looking to avoid attempts to build this test
> outside kernel tree.

Ok, I understood what you mean.
You want to remove ion test from kselftest build, but still maintain
the test code inside selftest.
Yes, I agree with this, kselftest will succeed (outside of kernel
source tree), and still somebody can build and run it locally.

But, my concern is, whole this recent patches were made to include it
into kselftest framework.
Otherwise in my previous patch, I included it under:
tools/testing/android (outside of selftest).

If, removing the android/ion from kselftest is fine (as of now), I can
push the new changes by tomorrow.
Just waiting for Greg and Laura to confirm...

Thanks,
Pintu

>
> thanks,
> -- Shuah