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

[Pintu Kumar]

On Fri, Nov 3, 2017 at 6:02 AM, Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> wrote:
> On 11/02/2017 05:54 PM, Laura Abbott wrote:
>> On 11/02/2017 10:53 AM, Shuah Khan wrote:
>>> 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.

Yes, this was the plan to create android folder there. Now other
android tests can also be added there.

>>>
>>> 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
>>>
>>
>> Yes, I think this is okay.
>>
>> Thanks,
>> Laura
>>
>
> Applied to linux-kselftest next for 4.15-rc1.
>

Great!
Thanks all for your reviews, comments and suggestions.
It was nice working with you all!

Looking forward for many more contributions :)


Thanks,
Pintu

> thanks,
> -- Shuah