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

[Shuah Khan]

On 11/02/2017 10:19 AM, Shuah Khan wrote:
> On 11/02/2017 10:15 AM, Pintu Kumar wrote:
>> 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.
> 
> Right.
> 
>>
>> 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).
> 
> I do want this test under kselftest framework. It doesn't make sense to
> add this under tools/testing/adroid. At some point we could add it to
> the selftest/Makefile TARGETS list for it to be run very easily if it
> resides under selftests.
> 
>>
>> 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...
>>

I did some sanity tests on this patch on a system with staging and android
enabled. Most users will see

ion_test.sh: No /dev/ion device found
ion_test.sh: May be CONFIG_ION is not set

Leaving this in kselftest run might be useful on adroid veification
tests.

With ion.h local file, outside builds work. We will just have to remember
to clean this up if and when ion moves from staging.

I am planning to take this patch as is for 4.15-rc1.

Laura! Are you good with the heap size changes in this patch?

thanks,
-- Shuah