by Shuah Khan

[permalink] [raw]

Subject: Re: [PATCH v2 0/7] Tests for sync infrastructure

On 12/01/2016 06:17 PM, Shuah Khan wrote:
> On 10/19/2016 06:49 AM, Emilio López wrote:
>> Hello everyone,
>>
>> This is a series of tests to exercise the sync kernel infrastructure. It is
>> meant to be a test suite for the work Gustavo has been doing to destage it.
>>
>> These tests were originally part of a battery of tests shipping with
>> Android's libsync that were rewritten to use the new userspace interfaces.
>>
>> This is the second iteration of the test suite. Main changes over v1 are
>> a reworked Makefile and small code style fixes.
>>
>> If you are testing this on v4.9-rc1, do note that the last test will
>> currently fail due to a regression[0].
>
> Hi Emilio,
>
> Thanks. I will apply these to linux-kselftest next for 4.10-rc1
>
> -- Shuah
>>
>> As usual, all comments are welcome.
>>

Hi Emilio,

Applied to linux-kselftest next. Could you take a look at the output
and see if it can be refined. Does [BAD] mean the test failed? Results
could refined to help user understand if a test failed or not clearly.
This can be done in a separate patch as a fix in one of the 4.01-rcs

thanks,
-- Shuah

2016-12-13 16:47:57

by Shuah Khan

[permalink] [raw]

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

Hi Gustavo,

On 11/01/2016 10:18 AM, Gustavo Padovan wrote:
> Hi Emilio,
>
> 2016-10-19 Emilio L?pez <[email protected]>:
>
>> 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 <[email protected]>
>> ---
>> 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