Re: [PATCH v2 1/7] selftest: sync: basic tests for sw_sync framework

[Gustavo Padovan <gustavo.padovan@collabora.com>]

Hi Emilio,

2016-10-19 Emilio López <emilio.lopez@collabora.co.uk>:

> These tests are based on the libsync test suite from Android.
> This commit lays the ground for future tests, as well as includes
> tests for a variety of basic allocation commands.
> 
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> ---
>  tools/testing/selftests/Makefile          |   1 +
>  tools/testing/selftests/sync/.gitignore   |   1 +
>  tools/testing/selftests/sync/Makefile     |  18 +++
>  tools/testing/selftests/sync/sw_sync.h    |  46 +++++++
>  tools/testing/selftests/sync/sync.c       | 221 ++++++++++++++++++++++++++++++
>  tools/testing/selftests/sync/sync.h       |  40 ++++++
>  tools/testing/selftests/sync/sync_alloc.c |  74 ++++++++++
>  tools/testing/selftests/sync/sync_test.c  |  71 ++++++++++
>  tools/testing/selftests/sync/synctest.h   |  47 +++++++
>  9 files changed, 519 insertions(+)
>  create mode 100644 tools/testing/selftests/sync/.gitignore
>  create mode 100644 tools/testing/selftests/sync/Makefile
>  create mode 100644 tools/testing/selftests/sync/sw_sync.h
>  create mode 100644 tools/testing/selftests/sync/sync.c
>  create mode 100644 tools/testing/selftests/sync/sync.h
>  create mode 100644 tools/testing/selftests/sync/sync_alloc.c
>  create mode 100644 tools/testing/selftests/sync/sync_test.c
>  create mode 100644 tools/testing/selftests/sync/synctest.h
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index f770dba..69cf1a6 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -23,6 +23,7 @@ TARGETS += seccomp
>  TARGETS += sigaltstack
>  TARGETS += size
>  TARGETS += static_keys
> +TARGETS += sync
>  TARGETS += sysctl
>  ifneq (1, $(quicktest))
>  TARGETS += timers
> diff --git a/tools/testing/selftests/sync/.gitignore b/tools/testing/selftests/sync/.gitignore
> new file mode 100644
> index 0000000..f5091e7
> --- /dev/null
> +++ b/tools/testing/selftests/sync/.gitignore
> @@ -0,0 +1 @@
> +sync_test
> diff --git a/tools/testing/selftests/sync/Makefile b/tools/testing/selftests/sync/Makefile
> new file mode 100644
> index 0000000..620a59a
> --- /dev/null
> +++ b/tools/testing/selftests/sync/Makefile
> @@ -0,0 +1,18 @@
> +CFLAGS += -O2 -g -std=gnu89 -pthread -Wall -Wextra
> +CFLAGS += -I../../../../usr/include/
> +LDFLAGS += -pthread
> +
> +TEST_PROGS = sync_test
> +
> +all: $(TEST_PROGS)
> +
> +include ../lib.mk
> +
> +OBJS = sync_test.o sync.o
> +
> +TESTS += sync_alloc.o
> +
> +sync_test: $(OBJS) $(TESTS)
> +
> +clean:
> +	$(RM) sync_test $(OBJS) $(TESTS)
> diff --git a/tools/testing/selftests/sync/sw_sync.h b/tools/testing/selftests/sync/sw_sync.h
> new file mode 100644
> index 0000000..e2cfc6ba
> --- /dev/null
> +++ b/tools/testing/selftests/sync/sw_sync.h
> @@ -0,0 +1,46 @@
> +/*
> + *  sw_sync abstraction
> + *
> + *  Copyright 2015-2016 Collabora Ltd.
> + *
> + *  Based on the implementation from the Android Open Source Project,
> + *
> + *  Copyright 2013 Google, Inc
> + *
> + *  Permission is hereby granted, free of charge, to any person obtaining a
> + *  copy of this software and associated documentation files (the "Software"),
> + *  to deal in the Software without restriction, including without limitation
> + *  the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + *  and/or sell copies of the Software, and to permit persons to whom the
> + *  Software is furnished to do so, subject to the following conditions:
> + *
> + *  The above copyright notice and this permission notice shall be included in
> + *  all copies or substantial portions of the Software.
> + *
> + *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + *  THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + *  OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + *  ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *  OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef SELFTESTS_SW_SYNC_H
> +#define SELFTESTS_SW_SYNC_H
> +
> +/*
> + * sw_sync is mainly intended for testing and should not be compiled into
> + * production kernels
> + */
> +
> +int sw_sync_timeline_create(void);
> +int sw_sync_timeline_is_valid(int fd);
> +int sw_sync_timeline_inc(int fd, unsigned int count);
> +void sw_sync_timeline_destroy(int fd);
> +
> +int sw_sync_fence_create(int fd, const char *name, unsigned int value);
> +int sw_sync_fence_is_valid(int fd);
> +void sw_sync_fence_destroy(int fd);
> +
> +#endif
> diff --git a/tools/testing/selftests/sync/sync.c b/tools/testing/selftests/sync/sync.c
> new file mode 100644
> index 0000000..f3d599f
> --- /dev/null
> +++ b/tools/testing/selftests/sync/sync.c
> @@ -0,0 +1,221 @@
> +/*
> + *  sync / sw_sync abstraction
> + *  Copyright 2015-2016 Collabora Ltd.
> + *
> + *  Based on the implementation from the Android Open Source Project,
> + *
> + *  Copyright 2012 Google, Inc
> + *
> + *  Permission is hereby granted, free of charge, to any person obtaining a
> + *  copy of this software and associated documentation files (the "Software"),
> + *  to deal in the Software without restriction, including without limitation
> + *  the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + *  and/or sell copies of the Software, and to permit persons to whom the
> + *  Software is furnished to do so, subject to the following conditions:
> + *
> + *  The above copyright notice and this permission notice shall be included in
> + *  all copies or substantial portions of the Software.
> + *
> + *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + *  THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + *  OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + *  ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *  OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <fcntl.h>
> +#include <malloc.h>
> +#include <poll.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#include "sync.h"
> +#include "sw_sync.h"
> +
> +#include <linux/sync_file.h>
> +
> +
> +/* SW_SYNC ioctls */
> +struct sw_sync_create_fence_data {
> +	__u32	value;
> +	char	name[32];
> +	__s32	fence;
> +};
> +
> +#define SW_SYNC_IOC_MAGIC		'W'
> +#define SW_SYNC_IOC_CREATE_FENCE	_IOWR(SW_SYNC_IOC_MAGIC, 0,\
> +					      struct sw_sync_create_fence_data)
> +#define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
> +
> +
> +int sync_wait(int fd, int timeout)
> +{
> +	struct pollfd fds;
> +
> +	fds.fd = fd;
> +	fds.events = POLLIN | POLLERR;
> +
> +	return poll(&fds, 1, timeout);
> +}

I think it is a good idea to keep the sync_wait behaviour similar to
what it was on android libsync. It should be something like this:

int sw_sync_wait(int fd, int timeout)                                           
{                                                                               
        struct pollfd fds = {0};                                                
        int ret;                                                                
                                                                                
        fds.fd = fd;                                                            
        fds.events = POLLIN;                                                    
                                                                                
        do {                                                                    
                 ret = poll(&fds, 1, timeout);                                  
                 if (ret > 0) {                                                 
                          if (fds.revents & (POLLERR | POLLNVAL)) {             
                                   errno = EINVAL;                              
                                   return -1;                                   
                          }                                                     
                          return 0;                                             
                 } else if (ret == 0) {                                         
                          errno = ETIME;                                        
                          return -1;                                            
                 }                                                              
        } while (ret == -1 && (errno == EINTR || errno == EAGAIN));             
                                                                                
        return ret;                                                             
} 


Gustavo