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

[Pintu Kumar]

On Wed, Oct 18, 2017 at 2:28 AM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
> On 10/17/2017 02:21 PM, Laura Abbott wrote:
>> On 10/14/2017 04:36 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 and CMA 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           |  44 ++++
>>>  tools/testing/selftests/android/ion/.gitignore     |   2 +
>>>  tools/testing/selftests/android/ion/Makefile       |  16 ++
>>>  tools/testing/selftests/android/ion/README         | 132 +++++++++++
>>>  tools/testing/selftests/android/ion/config         |   3 +
>>>  tools/testing/selftests/android/ion/ion_test.sh    |  61 +++++
>>>  .../testing/selftests/android/ion/ionapp_export.c  | 151 ++++++++++++
>>>  .../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 +
>>>  14 files changed, 1078 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 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
>>> +TARGETS += bpf
>>>  TARGETS += breakpoints
>>>  TARGETS += capabilities
>>>  TARGETS += cpufreq
>>> diff --git a/tools/testing/selftests/android/Makefile b/tools/testing/selftests/android/Makefile
>>> new file mode 100644
>>> index 0000000..ee76446
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/Makefile
>>> @@ -0,0 +1,44 @@
>>> +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 $@;\
>>> +            if [ -e $$DIR/$(TEST_PROGS) ]; then
>>> +                    rsync -a $$DIR/$(TEST_PROGS) $$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
>>> diff --git a/tools/testing/selftests/android/ion/.gitignore b/tools/testing/selftests/android/ion/.gitignore
>>> new file mode 100644
>>> index 0000000..67e6f39
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/.gitignore
>>> @@ -0,0 +1,2 @@
>>> +ionapp_export
>>> +ionapp_import
>>> diff --git a/tools/testing/selftests/android/ion/Makefile b/tools/testing/selftests/android/ion/Makefile
>>> new file mode 100644
>>> index 0000000..b84e3b1
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/Makefile
>>> @@ -0,0 +1,16 @@
>>> +
>>> +INCLUDEDIR := -I../../../../../drivers/staging/android/uapi/
>>> +CFLAGS := $(INCLUDEDIR) -Wall -O2 -g
>>> +
>>> +TEST_GEN_FILES := ionapp_export ionapp_import
>>> +
>>> +all: $(TEST_GEN_FILES)
>>> +
>>> +$(TEST_GEN_FILES): ipcsocket.c ionutils.c
>>> +
>>> +TEST_PROGS := ion_test.sh
>>> +
>>> +include ../../lib.mk
>>> +
>>> +$(OUTPUT)/ionapp_export: ionapp_export.c ipcsocket.c ionutils.c
>>> +$(OUTPUT)/ionapp_import: ionapp_import.c ipcsocket.c ionutils.c
>>> diff --git a/tools/testing/selftests/android/ion/README b/tools/testing/selftests/android/ion/README
>>> new file mode 100644
>>> index 0000000..163e353
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/README
>>> @@ -0,0 +1,132 @@
>>> +ION BUFFER SHARING UTILITY
>>> +==========================
>>> +File: ion_test.sh : Utility to test ION driver buffer sharing mechanism.
>>> +Author: Pintu Kumar <pintu.ping@xxxxxxxxx>
>>> +
>>> +Introduction:
>>> +-------------
>>> +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.
>>> +The following heap types are supported by ION driver.
>>> +ION_HEAP_TYPE_SYSTEM (0)
>>> +ION_HEAP_TYPE_SYSTEM_CONTIG (1)
>>> +ION_HEAP_TYPE_CARVEOUT (2)
>>> +ION_HEAP_TYPE_CHUNK (3)
>>> +ION_HEAP_TYPE_DMA (4)
>>> +
>>> +By default only the SYSTEM and SYSTEM_CONTIG heaps are supported.
>>> +Each heap is associated with the respective heap id.
>>> +This utility is designed in the form of client/server program.
>>> +The server part (ionapp_export) is the exporter of the buffer.
>>> +It is responsible for creating an ION client, allocating the buffer based on
>>> +the heap id, writing some data to this buffer and then exporting the FD
>>> +(associated with this buffer) to another process using socket IPC.
>>> +This FD is called as buffer FD (which is different than the ION client FD).
>>> +
>>> +The client part (ionapp_import) is the importer of the buffer.
>>> +It retrives the FD from the socket data and installs into its address space.
>>> +This new FD internally points to the same kernel buffer.
>>> +So first it reads the data that is stored in this buffer and prints it.
>>> +Then it writes the different size of data (it could be different data) to the
>>> +same buffer.
>>> +Finally the buffer FD must be closed by both the exporter and importer.
>>> +Thus the same kernel buffer is shared among two user space processes using
>>> +ION driver and only one time allocation.
>>> +
>>> +Prerequisite:
>>> +-------------
>>> +This utility works only if /dev/ion interface is present.
>>> +The following configs needs to be enabled in kernel to include ion driver.
>>> +CONFIG_ANDROID=y
>>> +CONFIG_ION=y
>>> +CONFIG_ION_SYSTEM_HEAP=y
>>
>> You also need CONFIG_STAGING right now as well.
>

Ok, added CONFIG_STAGING under config and README

> In which case, please make sure the test fails gracefully when the
> these config options are disabled.
>
> What does the test do when all of these options are disabled?
>
I assume that if these configs are not present the /dev/ion will also
not exists.
If that is the case then, I don't proceed with the test.
This is checked under ion_test script using: check_device

>>
>>> +
>>> +This utility requires to be run as root user.
>>> +
>>> +
>>> +Compile and test:
>>> +-----------------
>>> +This utility is made to be run as part of kselftest framework in kernel.
>>> +To compile and run using kselftest you can simply do the following from the
>>> +kernel top directory.
>>> +linux$ make TARGETS=android kselftest
>
> Please make sure
>
> make O=/tmp/kselftest TARGETS=android kselftest
>
> works.
>

Ok when I specify O as output directory. It did not work.
./run.sh: 3: ./run.sh: ./ion_test.sh: not found
../lib.mk:41: recipe for target 'run_tests' failed

When I checked /tmp/kselftest/ion/, I see that ion_test.sh is not installed.
Only the executable are installed.
I followed the same Makefile as futex.
Currently I am trying to figure out what could be the problem.
Any hint will be really appreciated.

>>> +Or you can also use:
>>> +linux$ make -C tools/testing/selftests TARGETS=android run_tests
>>> +Using the selftest it can directly execute the ion_test.sh script to test the
>>> +buffer sharing using ion system heap.
>>> +Currently the heap size is hard coded as just 10 bytes inside this script.
>>> +You need to be a root user to run under selftest.
>>> +
>>> +You can also compile and test manually using the following steps:
>>> +ion$ make
>>> +These will generate 2 executable: ionapp_export, ionapp_import
>>> +Now you can run the export and import manually by specifying the heap type
>>> +and the heap size.
>>> +You can also directly execute the shell script to run the test automatically.
>>> +Simply use the following command to run the test.
>>> +ion$ sudo ./ion_test.sh
>>> +
>>> +Test Results:
>>> +-------------
>>> +The utility is verified on Ubuntu-32 bit system with Linux Kernel 4.14.
>>> +Here is the snapshot of the test result using kselftest.
>>> +
>>> +linux# make TARGETS=android kselftest
>>> +make[2]: Entering directory '/home/pintu/PINTU/OPEN_SOURCE/KERNEL/
>
> Please don't include directory information in the README.
>

Ok done. Sorry about it

>>> +linux/tools/testing/selftests/android'
>>> +make[3]: Entering directory '/home/pintu/PINTU/OPEN_SOURCE/KERNEL/
>>> +linux/tools/testing/selftests/android/ion'
>>> +gcc -I../../../../../drivers/staging/android/uapi/ -Wall -O2 -g
>
> Please don't include gcc output information in the README.
>

done

>>> +ionapp_export.c ipcsocket.c ionutils.c   -o ionapp_export
>>> +gcc -I../../../../../drivers/staging/android/uapi/ -Wall -O2 -g
>>> +ionapp_import.c ipcsocket.c ionutils.c   -o ionapp_import
>>> +
>
> Include just the test results. Sorry I didn't catch this before.
>

Ok done. Just included only one result.

>>> +heap_type: 0, heap_size: 10
>>> +--------------------------------------
>>> +heap type: 0
>>> +  heap id: 1
>>> +heap name: ion_system_heap
>>> +--------------------------------------
>>> +Fill buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +Sharing fd: 6, Client fd: 5
>>> +<ion_close_buffer_fd>: buffer release successfully....
>>> +Received buffer fd: 4
>>> +Read buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0x0 0x0 0x0 0x0 0x0 0x0
>>> +0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
>>> +Fill buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +0xfd 0xfd
>>> +<ion_close_buffer_fd>: buffer release successfully....
>>> +ion_test.sh: heap_type: 0 - [PASS]
>>> +
>>> +heap_type: 1, heap_size: 10
>>> +--------------------------------------
>>> +heap type: 1
>>> +  heap id: 0
>>> +heap name: ion_system_contig_heap
>>> +--------------------------------------
>>> +Fill buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +Sharing fd: 6, Client fd: 5
>>> +<ion_close_buffer_fd>: buffer release successfully....
>>> +Received buffer fd: 4
>>> +Read buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0x0 0x0 0x0 0x0 0x0 0x0 0x0
>>> +0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0
>>> +Fill buffer content:
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd 0xfd
>>> +0xfd 0xfd
>>> +<ion_close_buffer_fd>: buffer release successfully....
>>> +ion_test.sh: heap_type: 1 - [PASS]
>>> +
>>> +ion_test.sh: done
>>> +make[2]: Leaving directory '/home/pintu/PINTU/OPEN_SOURCE/KERNEL/
>>> +linux/tools/testing/selftests/android'
>>> diff --git a/tools/testing/selftests/android/ion/config b/tools/testing/selftests/android/ion/config
>>> new file mode 100644
>>> index 0000000..614b615
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/config
>>> @@ -0,0 +1,3 @@
>>> +CONFIG_ANDROID=y
>>> +CONFIG_ION=y
>>> +CONFIG_ION_SYSTEM_HEAP=y
>>> diff --git a/tools/testing/selftests/android/ion/ion_test.sh b/tools/testing/selftests/android/ion/ion_test.sh
>>> new file mode 100755
>>> index 0000000..dc77a39
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/ion_test.sh
>>> @@ -0,0 +1,61 @@
>>> +#!/bin/bash
>>> +
>>> +heapsize=10
>>
>> Nearly all the heaps are going to operate on page size granularity,
>> just set this to 4096.
>>

Ok changed to 4096


>>> +TCID="ion_test.sh"
>>> +errcode=0
>>> +
>>> +run_test()
>>> +{
>>> +    heaptype=$1
>>> +    ./ionapp_export -i $heaptype -s $heapsize &
>>> +    sleep 1
>>> +    ./ionapp_import
>>> +    if [ $? -ne 0 ]; then
>>> +            echo "$TCID: heap_type: $heaptype - [FAIL]"
>>> +            errcode=1
>>> +    else
>>> +            echo "$TCID: heap_type: $heaptype - [PASS]"
>>> +    fi
>>> +    sleep 1
>>> +    echo ""
>>> +}
>>> +
>>> +check_root()
>>> +{
>>> +    uid=$(id -u)
>>> +    if [ $uid -ne 0 ]; then
>>> +            echo $TCID: must be run as root >&2
>>> +            exit 0
>>> +    fi
>>> +}
>>> +
>>> +check_device()
>>> +{
>>> +    DEVICE=/dev/ion
>>> +    if [ ! -e $DEVICE ]; then
>>> +            echo $TCID: No $DEVICE device found >&2
>>> +            echo $TCID: May be CONFIG_ION is not set >&2
>>> +            exit 0
>>> +    fi
>>> +}
>>> +
>>> +main_function()
>>> +{
>>> +    check_device
>>> +    check_root
>>> +
>>> +    # ION_SYSTEM_HEAP TEST
>>> +    run_test 0
>>> +    # ION_SYSTEM_CONTIG_HEAP TEST
>>> +    run_test 1
>>> +    # ION_CARVEOUT HEAP TEST
>>> +    #run_test 2
>>> +    # ION_CHUNK_HEAP TEST
>>> +    #run_test 3
>>> +    # ION_CMA_HEAP TEST
>>> +    #run_test 4
>>> +}
>>> +
>>> +main_function
>>> +echo "$TCID: done"
>>> +exit $errcode
>>> diff --git a/tools/testing/selftests/android/ion/ionapp_export.c b/tools/testing/selftests/android/ion/ionapp_export.c
>>> new file mode 100644
>>> index 0000000..59b434f
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/android/ion/ionapp_export.c
>>> @@ -0,0 +1,151 @@
>>> +/*
>>> + * ionapp_export.c
>>> + *
>>> + * It is a user space utility to create and export android
>>> + * ion memory buffer fd to another process using unix domain socket as IPC.
>>> + * This acts like a server for ionapp_import(client).
>>> + * So, this server has to be started first before the client.
>>> + *
>>> + * Copyright (C) 2017 Pintu Kumar <pintu.ping@xxxxxxxxx>
>>> + *
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <string.h>
>>> +#include <unistd.h>
>>> +#include <errno.h>
>>> +#include <sys/time.h>
>>> +#include "ionutils.h"
>>> +#include "ipcsocket.h"
>>> +
>>> +
>>> +void print_usage(int argc, char *argv[])
>>> +{
>>> +    printf("********** HEAP TYPE ***********\n");
>>> +    printf("0: ION_HEAP_TYPE_SYSTEM\n");
>>> +    printf("1: ION_HEAP_TYPE_SYSTEM_CONTIG\n");
>>> +    printf("2: ION_HEAP_TYPE_CARVEOUT\n");
>>> +    printf("3: ION_HEAP_TYPE_CHUNK\n");
>>> +    printf("4: ION_HEAP_TYPE_DMA\n");
>>> +
>>> +    printf("Usage: %s [-h <help>] [-i <heap id>] [-s <size in bytes>]\n",
>>> +            argv[0]);
>>> +}> +
>>
>> Again, there is no need to test multiple heaps. Just
>> do the test with system heap and remove all the
>> other ones.
>>

Ok removed all the options from here. Now heap type is passed from the script.

Thanks for the review.
I will be submitting the new patch set soon.


>> Thanks,
>> Laura
>>
>>
>
> thanks,
> -- Shuah