Date:   Wed, 4 Oct 2017 16:59:17 +0530
From:   Pintu Kumar <pintu.ping@...il.com>
To:     Laura Abbott <labbott@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Pintu Kumar <pintu_agarwal@...oo.com>
Subject: Re: [PATCHv2 1/1] [tools]: android/ion: userspace test utility for
 ion buffer sharing

On Wed, Oct 4, 2017 at 5:42 AM, Laura Abbott <labbott@...hat.com> wrote:
> On 10/03/2017 09:48 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 as IPC to transfer an FD to another process
>> and install it.
>>
>> This utility demonstrates how ION buffer sharing can be implemented between
>> two user space processes, using various heap ids.
>>
>> This utility is verified on Ubuntu 32-bit machine using 2 independent
>> process such as: ionapp_export (server) and ionapp_import (client).
>> First the server needs to be run to export FD to the client.
>> This utility works only if /dev/ion interface is present.
>>
>> Here is a sample demo example:
>>
>> linux/tools/android/ion$ sudo ./ionapp_export.out -i 1 -s 10
>> heap_type: 2, heap_size: 10
>> 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....
>>
>> linux/tools/android/ion$ sudo ./ionapp_import.out
>> 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....
>>
>> Signed-off-by: Pintu Agarwal <pintu.ping@...il.com>
>> ---
>>  tools/android/ion/Makefile        |  17 +++
>>  tools/android/ion/ionapp_export.c | 151 ++++++++++++++++++++++++
>>  tools/android/ion/ionapp_import.c |  86 ++++++++++++++
>>  tools/android/ion/ionutils.c      | 239 ++++++++++++++++++++++++++++++++++++++
>>  tools/android/ion/ionutils.h      |  54 +++++++++
>>  tools/android/ion/ipcsocket.c     | 227 ++++++++++++++++++++++++++++++++++++
>>  tools/android/ion/ipcsocket.h     |  35 ++++++
>>  7 files changed, 809 insertions(+)
>>  create mode 100644 tools/android/ion/Makefile
>>  create mode 100644 tools/android/ion/ionapp_export.c
>>  create mode 100644 tools/android/ion/ionapp_import.c
>>  create mode 100644 tools/android/ion/ionutils.c
>>  create mode 100644 tools/android/ion/ionutils.h
>>  create mode 100644 tools/android/ion/ipcsocket.c
>>  create mode 100644 tools/android/ion/ipcsocket.h
>>
>> diff --git a/tools/android/ion/Makefile b/tools/android/ion/Makefile
>> new file mode 100644
>> index 0000000..57d2e98
>> --- /dev/null
>> +++ b/tools/android/ion/Makefile
>> @@ -0,0 +1,17 @@
>> +
>> +CC := $(CROSS_COMPILE)gcc
>> +
>> +INCLUDEDIR := -I../../../drivers/staging/android/uapi/
>> +
>> +CCFLAGS := $(INCLUDEDIR) -Wall -O2 -g
>> +
>> +all: ionapp_export ionapp_import
>> +
>> +ionapp_import : ionapp_import.c
>> +     $(CC) -o ionapp_import.out ionapp_import.c ipcsocket.c ionutils.c $(CCFLAGS)
>> +
>> +ionapp_export : ionapp_export.c
>> +     $(CC) -o ionapp_export.out ionapp_export.c ipcsocket.c ionutils.c $(CCFLAGS)
>> +
>
> Having to run the two apps separately is a pain,
> can you combine the two into one application and use fork?
>
The whole thing about this test is to share an FD over 2 independent processes.
I think sharing an FD using fork() [parent/child] is not a real use
case scenarios.
Some people may not like the fork example.
Initially when I started with ION, I also needed an FD sharing mechanism between
2 different process. Thus I came up with this framework using ipcsocket.
Later, if required, we can even replace this with binder_ipc for
android use cases.
Anyways, binder_ipc also internally uses the same concept as this ipcsocket.
To reduce the pain, we can invoke both the tests from a single shell scripts.
I will try to include the same in kselftests, if possible.

If fork example is really required, we can add another test for it.
This is my opinion.

>> +clean:
>> +     rm -rf *.o *~ *.out
>> diff --git a/tools/android/ion/ionapp_export.c b/tools/android/ion/ionapp_export.c
>> new file mode 100644
>> index 0000000..74b369b
>> --- /dev/null
>> +++ b/tools/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@...il.com>
>> + *
>> + * 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 ID ***********\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]);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +     int opt, ret, status, heapid;
>> +     int sockfd, client_fd, shared_fd;
>> +     unsigned char *map_buf;
>> +     unsigned long map_len, heap_type, heap_size, flags;
>> +     struct ion_buffer_info info;
>> +     struct socket_info skinfo;
>> +
>> +     if (argc < 2) {
>> +             print_usage(argc, argv);
>> +             return -1;
>> +     }
>> +
>> +     heap_size = 0;
>> +     flags = 0;
>> +
>> +     while ((opt = getopt(argc, argv, "hi:s:")) != -1) {
>> +             switch (opt) {
>> +             case 'h':
>> +                     print_usage(argc, argv);
>> +                     exit(0);
>> +                     break;
>> +             case 'i':
>> +                     heapid = atoi(optarg);
>> +                     switch (heapid) {
>> +                     case 0:
>> +                             heap_type = 1 << ION_HEAP_TYPE_SYSTEM;
>> +                             break;
>> +                     case 1:
>> +                             heap_type = 1 << ION_HEAP_TYPE_SYSTEM_CONTIG;
>> +                             break;
>> +                     case 2:
>> +                             heap_type = 1 << ION_HEAP_TYPE_CARVEOUT;
>> +                             break;
>> +                     case 3:
>> +                             heap_type = 1 << ION_HEAP_TYPE_CHUNK;
>> +                             break;
>> +                     case 4:
>> +                             heap_type = 1 << ION_HEAP_TYPE_DMA;
>> +                             break;
>> +                     default:
>> +                             printf("ERROR: Wrong - heap type\n");
>> +                             exit(1);
>> +                     }
>> +                     break;
>> +             case 's':
>> +                     heap_size = atoi(optarg);
>> +                     break;
>> +             default:
>> +                     print_usage(argc, argv);
>> +                     exit(1);
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (heap_size <= 0) {
>> +             printf("heap_size cannot be 0\n");
>> +             print_usage(argc, argv);
>> +             exit(1);
>> +     }
>> +
>> +     printf("heap_type: %ld, heap_size: %ld\n", heap_type, heap_size);
>> +     info.heap_type = heap_type;
>> +     info.heap_size = heap_size;
>> +     info.flag_type = flags;
>> +
>> +     /* This is server: open the socket connection first */
>> +     /* Here; 1 indicates server or exporter */
>> +     status = opensocket(&sockfd, SOCKET_NAME, 1);
>> +     if (status < 0) {
>> +             fprintf(stderr, "<%s>: Failed opensocket.\n", __func__);
>> +             goto err_socket;
>> +     }
>> +     skinfo.sockfd = sockfd;
>> +
>> +     ret = ion_export_buffer_fd(&info);
>> +     if (ret < 0) {
>> +             fprintf(stderr, "FAILED: ion_get_buffer_fd\n");
>> +             goto err_export;
>> +     }
>> +     client_fd = info.ionfd;
>> +     shared_fd = info.buffd;
>> +     map_buf = info.buffer;
>> +     map_len = info.buflen;
>> +     write_buffer(map_buf, map_len);
>> +
>> +     /* share ion buf fd with other user process */
>> +     printf("Sharing fd: %d, Client fd: %d\n", shared_fd, client_fd);
>> +     skinfo.datafd = shared_fd;
>> +     skinfo.buflen = map_len;
>> +
>> +     ret = socket_send_fd(&skinfo);
>> +     if (ret < 0) {
>> +             fprintf(stderr, "FAILED: socket_send_fd\n");
>> +             goto err_send;
>> +     }
>> +
>> +err_send:
>> +err_export:
>> +     ion_close_buffer_fd(&info);
>> +
>> +err_socket:
>> +     closesocket(sockfd, SOCKET_NAME);
>> +
>> +     return 0;
>> +}
>> diff --git a/tools/android/ion/ionapp_import.c b/tools/android/ion/ionapp_import.c
>> new file mode 100644
>> index 0000000..4d8646d
>> --- /dev/null
>> +++ b/tools/android/ion/ionapp_import.c
>> @@ -0,0 +1,86 @@
>> +/*
>> + * ionapp_import.c
>> + *
>> + * It is a user space utility to receive android ion memory buffer fd
>> + * over unix domain socket IPC that can be exported by ionapp_export.
>> + * This acts like a client for ionapp_export.
>> + *
>> + * Copyright (C) 2017 Pintu Kumar <pintu.ping@...il.com>
>> + *
>> + * 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 <unistd.h>
>> +#include <string.h>
>> +#include "ionutils.h"
>> +#include "ipcsocket.h"
>> +
>> +
>> +int main(void)
>> +{
>> +     int ret, status;
>> +     int sockfd, shared_fd;
>> +     unsigned char *map_buf;
>> +     unsigned long map_len;
>> +     struct ion_buffer_info info;
>> +     struct socket_info skinfo;
>> +
>> +     /* This is the client part. Here 0 means client or importer */
>> +     status = opensocket(&sockfd, SOCKET_NAME, 0);
>> +     if (status < 0) {
>> +             fprintf(stderr, "No exporter exists...\n");
>> +             goto err_socket;
>> +     }
>> +
>> +     skinfo.sockfd = sockfd;
>> +
>> +     ret = socket_receive_fd(&skinfo);
>> +     if (ret < 0) {
>> +             fprintf(stderr, "Failed: socket_receive_fd\n");
>> +             goto err_recv;
>> +     }
>> +
>> +     shared_fd = skinfo.datafd;
>> +     printf("Received buffer fd: %d\n", shared_fd);
>> +     if (shared_fd <= 0) {
>> +             fprintf(stderr, "ERROR: improper buf fd\n");
>> +             goto err_fd;
>> +     }
>> +
>> +     memset(&info, 0, sizeof(info));
>> +     info.buffd = shared_fd;
>> +     info.buflen = ION_BUFFER_LEN;
>> +
>> +     ret = ion_import_buffer_fd(&info);
>> +     if (ret < 0) {
>> +             fprintf(stderr, "Failed: ion_use_buffer_fd\n");
>> +             goto err_import;
>> +     }
>> +
>> +     map_buf = info.buffer;
>> +     map_len = info.buflen;
>> +     read_buffer(map_buf, map_len);
>> +
>> +     /* Write probably new data to the same buffer again */
>> +     map_len = ION_BUFFER_LEN;
>> +     write_buffer(map_buf, map_len);
>> +
>> +err_import:
>> +     ion_close_buffer_fd(&info);
>> +err_fd:
>> +err_recv:
>> +err_socket:
>> +     closesocket(sockfd, SOCKET_NAME);
>> +
>> +     return 0;
>> +}
>> diff --git a/tools/android/ion/ionutils.c b/tools/android/ion/ionutils.c
>> new file mode 100644
>> index 0000000..2d9012b
>> --- /dev/null
>> +++ b/tools/android/ion/ionutils.c
>> @@ -0,0 +1,239 @@
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <errno.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +#include "ionutils.h"
>> +#include "ipcsocket.h"
>> +
>> +
>> +void write_buffer(void *buffer, unsigned long len)
>> +{
>> +     int i;
>> +     unsigned char *ptr = (unsigned char *)buffer;
>> +
>> +     if (!ptr) {
>> +             fprintf(stderr, "<%s>: Invalid buffer...\n", __func__);
>> +             return;
>> +     }
>> +
>> +     printf("Fill buffer content:\n");
>> +     memset(ptr, 0xfd, len);
>> +     for (i = 0; i < len; i++)
>> +             printf("0x%x ", ptr[i]);
>> +     printf("\n");
>> +}
>> +
>> +void read_buffer(void *buffer, unsigned long len)
>> +{
>> +     int i;
>> +     unsigned char *ptr = (unsigned char *)buffer;
>> +
>> +     if (!ptr) {
>> +             fprintf(stderr, "<%s>: Invalid buffer...\n", __func__);
>> +             return;
>> +     }
>> +
>> +     printf("Read buffer content:\n");
>> +     for (i = 0; i < len; i++)
>> +             printf("0x%x ", ptr[i]);
>> +     printf("\n");
>> +}
>> +
>> +int ion_export_buffer_fd(struct ion_buffer_info *ion_info)
>> +{
>> +     int ret, ionfd, buffer_fd;
>> +     unsigned int heap_type, flag_type;
>> +     unsigned long heap_size, maplen;
>> +     unsigned char *map_buffer;
>> +     struct ion_allocation_data alloc_data;
>> +
>> +     if (!ion_info) {
>> +             fprintf(stderr, "<%s>: Invalid ion info\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     /* Create an ION client */
>> +     ionfd = open(ION_DEVICE, O_RDWR);
>> +     if (ionfd < 0) {
>> +             fprintf(stderr, "<%s>: Failed to open ion client: %s\n",
>> +                     __func__, strerror(errno));
>> +             return -1;
>> +     }
>> +
>> +     heap_type = ion_info->heap_type;
>> +     heap_size = ion_info->heap_size;
>> +     flag_type = ion_info->flag_type;
>> +     alloc_data.len = heap_size;
>> +     alloc_data.heap_id_mask = heap_type;
>
> The heap type and heap ID are not the same thing. You need
> to determine the heap id from using the new query ioctl.
>

Ok I will check how query ioctl can be used here.
Thanks

>> +     alloc_data.flags = flag_type;
>> +
>> +     /* Allocate memory for this ION client as per heap_type */
>> +     ret = ioctl(ionfd, ION_IOC_ALLOC, &alloc_data);
>> +     if (ret < 0) {
>> +             fprintf(stderr, "<%s>: Failed: ION_IOC_ALLOC: %s\n",
>> +                     __func__, strerror(errno));
>> +             goto err_alloc;
>> +     }
>> +
>> +     /* This will return a valid buffer fd */
>> +     buffer_fd = alloc_data.fd;
>> +     maplen = alloc_data.len;
>> +
>> +     if (buffer_fd <= 0 || maplen <= 0) {
>
> 0 is a valid file descriptor
>

Ok I will fix it.
Thanks

>> +             fprintf(stderr, "<%s>: Invalid map data, fd: %d, len: %ld\n",
>> +                     __func__, buffer_fd, maplen);
>> +             goto err_fd_data;
>> +     }
>> +
>> +     /* Create memory mapped buffer for the buffer fd */
>> +     map_buffer = (unsigned char *)mmap(NULL, maplen, PROT_READ|PROT_WRITE,
>> +                     MAP_SHARED, buffer_fd, 0);
>> +     if (ion_info->buffer == MAP_FAILED) {
>
> This should be checking map_buffer
>

Ok I will fix it.
Thanks

>> +             fprintf(stderr, "<%s>: Failed: mmap: %s\n",
>> +                     __func__, strerror(errno));
>> +             goto err_mmap;
>> +     }
>> +
>> +     ion_info->ionfd = ionfd;
>> +     ion_info->buffd = buffer_fd;
>> +     ion_info->buffer = map_buffer;
>> +     ion_info->buflen = maplen;
>> +
>> +     return 0;
>> +
>> +     munmap(map_buffer, maplen);
>> +
>> +err_fd_data:
>> +err_mmap:
>> +     /* in case of error: close the buffer fd */
>> +     if (buffer_fd > 0)
>> +             close(buffer_fd);
>> +
>> +err_alloc:
>> +     /* In case of error: close the ion client fd */
>> +     if (ionfd > 0)
>> +             close(ionfd);
>> +
>> +     return -1;
>> +}
>> +
>> +int ion_import_buffer_fd(struct ion_buffer_info *ion_info)
>> +{
>> +     int ionfd, buffd;
>> +     unsigned char *map_buf;
>> +     unsigned long map_len;
>> +
>> +     if (!ion_info) {
>> +             fprintf(stderr, "<%s>: Invalid ion info\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     ionfd = open(ION_DEVICE, O_RDWR);
>> +     if (ionfd < 0) {
>> +             fprintf(stderr, "<%s>: Failed to open ion client: %s\n",
>> +                     __func__, strerror(errno));
>> +             return -1;
>> +     }
>> +
>
> There's no need to open the ion device here since you can just use
> the dma_buf directly.
>

Ok got it. Thanks

>
>> +     map_len = ion_info->buflen;
>> +     buffd = ion_info->buffd;
>> +
>> +     if (buffd <= 0 || map_len <= 0) {
>
> 0 is a valid file descriptor
>

Ok I will fix it.
Thanks

>> +             fprintf(stderr, "<%s>: Invalid map data, fd: %d, len: %ld\n",
>> +                     __func__, buffd, map_len);
>> +             goto err_fd_data;
>> +     }
>> +
>> +     map_buf = (unsigned char *)mmap(NULL, map_len, PROT_READ|PROT_WRITE,
>> +                     MAP_SHARED, buffd, 0);
>> +     if (map_buf == MAP_FAILED) {
>> +             printf("<%s>: Failed - mmap: %s\n",
>> +                     __func__, strerror(errno));
>> +             goto err_mmap;
>> +     }
>> +
>> +     ion_info->ionfd = ionfd;
>> +     ion_info->buffd = buffd;
>> +     ion_info->buffer = map_buf;
>> +     ion_info->buflen = map_len;
>> +
>> +     return 0;
>> +
>> +err_mmap:
>> +     if (buffd)
>> +             close(buffd);
>> +
>> +err_fd_data:
>> +     if (ionfd)
>> +             close(ionfd);
>> +
>> +     return -1;
>> +}
>> +
>> +void ion_close_buffer_fd(struct ion_buffer_info *ion_info)
>> +{
>> +     if (ion_info) {
>> +             /* unmap the buffer properly in the end */
>> +             munmap(ion_info->buffer, ion_info->buflen);
>> +             /* close the buffer fd */
>> +             if (ion_info->buffd > 0)
>> +                     close(ion_info->buffd);
>> +             /* Finally, close the client fd */
>> +             if (ion_info->ionfd > 0)
>> +                     close(ion_info->ionfd);
>> +             printf("<%s>: buffer release successfully....\n", __func__);
>> +     }
>> +}
>> +
>> +int socket_send_fd(struct socket_info *info)
>> +{
>> +     int status;
>> +     int fd, sockfd;
>> +     struct socketdata skdata;
>> +
>> +     if (!info) {
>> +             fprintf(stderr, "<%s>: Invalid socket info\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     sockfd = info->sockfd;
>> +     fd = info->datafd;
>> +     memset(&skdata, 0, sizeof(skdata));
>> +     skdata.data = fd;
>> +     skdata.len = sizeof(skdata.data);
>> +     status = sendtosocket(sockfd, &skdata);
>> +     if (status < 0) {
>> +             fprintf(stderr, "<%s>: Failed: sendtosocket\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +int socket_receive_fd(struct socket_info *info)
>> +{
>> +     int status;
>> +     int fd, sockfd;
>> +     struct socketdata skdata;
>> +
>> +     if (!info) {
>> +             fprintf(stderr, "<%s>: Invalid socket info\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     sockfd = info->sockfd;
>> +     memset(&skdata, 0, sizeof(skdata));
>> +     status = receivefromsocket(sockfd, &skdata);
>> +     if (status < 0) {
>> +             fprintf(stderr, "<%s>: Failed: receivefromsocket\n", __func__);
>> +             return -1;
>> +     }
>> +
>> +     fd = (int)skdata.data;
>> +     info->datafd = fd;
>> +
>> +     return status;
>> +}
>
> Thanks,
> Laura

Powered by blists - more mailing lists