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

[Shuah Khan <shuah@kernel.org>]

Hi Gustavo,

On 11/01/2016 10:18 AM, Gustavo Padovan wrote:
> 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:

I didn't notice that this comment wasn't addressed. I will go ahead
and continue with the plan to get these into 4.10-rc1. If you want
to send me patch for this behavior, please do. We can address that
as a fix in 4.10-rc2

> 
> 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
> 

thanks,
-- Shuah