* [PATCH lttng-tools v4 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer Yannick Lamarre
` (9 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
lttng_poll_mod should call compat_(e)poll_mod.
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/common/compat/poll.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index d4bd87f5..fde54ddb 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -177,7 +177,7 @@ extern int compat_epoll_del(struct lttng_poll_event *events, int fd);
extern int compat_epoll_mod(struct lttng_poll_event *events,
int fd, uint32_t req_events);
#define lttng_poll_mod(events, fd, req_events) \
- compat_epoll_add(events, fd, req_events)
+ compat_epoll_mod(events, fd, req_events)
/*
* Set up the poll set limits variable poll_max_size
@@ -361,7 +361,7 @@ extern int compat_poll_del(struct lttng_poll_event *events, int fd);
extern int compat_poll_mod(struct lttng_poll_event *events,
int fd, uint32_t req_events);
#define lttng_poll_mod(events, fd, req_events) \
- compat_poll_add(events, fd, req_events)
+ compat_poll_mod(events, fd, req_events)
/*
* Set up the poll set limits variable poll_max_size
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
2019-04-01 17:59 ` [PATCH lttng-tools v4 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
` (8 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
.gitignore | 1 +
tests/unit/Makefile.am | 9 +-
tests/unit/test_utils_compat_poll.c | 236 ++++++++++++++++++++++++++++++++++++
3 files changed, 245 insertions(+), 1 deletion(-)
create mode 100644 tests/unit/test_utils_compat_poll.c
diff --git a/.gitignore b/.gitignore
index 19276012..b5d2a55a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,6 +73,7 @@ TAGS
/tests/unit/test_session
/tests/unit/test_uri
/tests/unit/test_ust_data
+/tests/unit/test_utils_compat_poll
/tests/unit/test_utils_parse_size_suffix
/tests/unit/test_utils_parse_time_suffix
/tests/unit/test_utils_expand_path
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index 5f485f00..bcda6a5c 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -12,6 +12,7 @@ TESTS = test_kernel_data \
test_utils_parse_size_suffix \
test_utils_parse_time_suffix \
test_utils_expand_path \
+ test_utils_compat_poll \
test_string_utils \
test_notification \
ini_config/test_ini_config
@@ -28,7 +29,8 @@ LIBLTTNG_CTL=$(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la
# Define test programs
noinst_PROGRAMS = test_uri test_session test_kernel_data \
test_utils_parse_size_suffix test_utils_parse_time_suffix \
- test_utils_expand_path test_string_utils test_notification
+ test_utils_expand_path test_utils_compat_poll \
+ test_string_utils test_notification
if HAVE_LIBLTTNG_UST_CTL
noinst_PROGRAMS += test_ust_data
@@ -145,6 +147,11 @@ test_utils_parse_size_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL
test_utils_parse_time_suffix_SOURCES = test_utils_parse_time_suffix.c
test_utils_parse_time_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
+# compat_poll unit test
+test_utils_compat_poll_SOURCES = test_utils_compat_poll.c
+test_utils_compat_poll_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(DL_LIBS) \
+ $(top_builddir)/src/common/compat/libcompat.la $(LIBCOMMON)
+
# expand_path unit test
test_utils_expand_path_SOURCES = test_utils_expand_path.c
test_utils_expand_path_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL_LIBS)
diff --git a/tests/unit/test_utils_compat_poll.c b/tests/unit/test_utils_compat_poll.c
new file mode 100644
index 00000000..f2c49254
--- /dev/null
+++ b/tests/unit/test_utils_compat_poll.c
@@ -0,0 +1,236 @@
+/*
+ * test_utils_compat_poll.c
+ *
+ * Unit tests for the compatibility layer of poll/epoll API.
+ *
+ * Copyright (C) 2019 Yannick Lamarre <ylamarre@efficios.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by as
+ * published by the Free Software Foundation; only version 2 of the License.
+ *
+ * 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 <assert.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <tap/tap.h>
+
+#include <common/compat/poll.h>
+#include <common/readwrite.h>
+
+/* Verification without trashing test order in the child process */
+#define childok(e, test, ...) do { \
+ if (!(e)) { \
+ diag(test, ## __VA_ARGS__); \
+ _exit(EXIT_FAILURE); \
+ } \
+} while(0)
+
+/* For error.h */
+int lttng_opt_quiet = 1;
+int lttng_opt_verbose;
+int lttng_opt_mi;
+
+#ifdef HAVE_EPOLL
+#define NUM_TESTS 46
+#else
+#define NUM_TESTS 45
+#endif
+
+#ifdef HAVE_EPOLL
+#if defined(HAVE_EPOLL_CREATE1) && defined(EPOLL_CLOEXEC)
+#define CLOE_VALUE EPOLL_CLOEXEC
+#else
+#define CLOE_VALUE FD_CLOEXEC
+#endif
+
+/* Non-zero 8-bits arbitrary value below 0x7f to ensure no sign extension
+ * used to verify that the value is properly propagated throught the pipe.
+ */
+#define MAGIC_VALUE ((char)0x5A)
+
+void test_epoll_compat(void)
+{
+ /*
+ * Type conversion present to disable warning of anonymous enum from
+ * compiler.
+ */
+ ok((int)LTTNG_CLOEXEC == (int)CLOE_VALUE, "epoll's CLOEXEC value");
+}
+#endif
+
+void test_alloc(void)
+{
+ struct lttng_poll_event poll_events;
+
+ lttng_poll_init(&poll_events);
+
+ /* Null pointer */
+ ok(lttng_poll_create(NULL, 1, 0) != 0, "Create over NULL pointer fails");
+ /* Size 0 */
+ ok(lttng_poll_create(&poll_events, 0, 0) != 0, "Create with size 0 fails");
+ /* without CLOEXEC */
+ ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
+ /*
+ * lttng_poll_event structure untested due to incompatibility across
+ * sublayers. lttng_poll_clean cannot be tested. There is no success
+ * criteria. Verify set's max size cases.
+ */
+ lttng_poll_clean(&poll_events);
+}
+
+/* Tests stuff related to what would be handled with epoll_ctl. */
+void test_add_del(void)
+{
+ struct lttng_poll_event poll_events;
+
+ lttng_poll_init(&poll_events);
+ ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
+ ok(lttng_poll_add(&poll_events, 1, LPOLLIN) != 0, "Adding to uninitialized structure fails");
+ ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
+
+ lttng_poll_create(&poll_events, 1, 0);
+ ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set created empty");
+
+ ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
+ ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
+
+ ok(lttng_poll_add(&poll_events, 1, LPOLLIN) == 0, "Adding valid FD succeeds");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Nb of elements incremented");
+
+ ok(lttng_poll_del(NULL, 1) != 0, "Removing from NULL set fails");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
+
+ ok(lttng_poll_del(&poll_events, -1) != 0, "Removing from negative FD fails");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
+
+ ok(lttng_poll_del(&poll_events, 2) == 0, "Removing invalid FD still succeeds");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of elements unchanged");
+
+ ok(lttng_poll_del(&poll_events, 1) == 0, "Removing valid FD succeeds");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements decremented");
+
+ ok(lttng_poll_del(&poll_events, 1) != 0, "Removing from empty set fails");
+ ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements unchanged");
+
+ lttng_poll_clean(&poll_events);
+}
+
+void test_mod_wait(void)
+{
+ struct lttng_poll_event poll_events;
+ struct lttng_poll_event cpoll_events;
+
+ int hupfd[2];
+ int infd[2];
+ pid_t cpid;
+ char rbuf = 0, tbuf = MAGIC_VALUE;
+ int wstatus;
+
+ lttng_poll_init(&poll_events);
+ lttng_poll_init(&cpoll_events);
+
+ ok(pipe(hupfd) != -1);
+ ok(pipe(infd) != -1);
+
+ cpid = fork();
+ if (cpid == 0) {
+ childok(lttng_poll_create(&cpoll_events, 1, 0) == 0, "Create valid poll set succeeds");
+ childok(lttng_poll_mod(NULL, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+ childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+ childok(lttng_poll_add(&cpoll_events, infd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
+ childok(lttng_poll_mod(&cpoll_events, -1, LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
+ childok(lttng_poll_mod(&cpoll_events, hupfd[0], LPOLLIN) == 0, "lttng_poll_mod on unincluded FD goes on");
+ childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == 0, "Modify event type succeeds");
+ childok(close(infd[1]) == 0, "Close valid FD succeeds");
+ childok(lttng_poll_wait(&cpoll_events, -1) == 1, "Wait on close times out");
+ childok(lttng_read(infd[0], &rbuf, 1) == 1, "Data is present in the pipe");
+ childok(rbuf == MAGIC_VALUE, "Received data is consistent with transmitted data");
+ childok(lttng_poll_del(&cpoll_events, infd[0]) == 0, "Removing valid FD succeeds");
+ childok(close(infd[0]) == 0, "Close valid FD succeeds");
+ childok(close(hupfd[0]) == 0, "Close valid FD succeeds");
+ childok(close(hupfd[1]) == 0, "Close valid FD succeeds");
+ lttng_poll_clean(&cpoll_events);
+ _exit(EXIT_SUCCESS);
+ } else {
+ ok(close(hupfd[1]) == 0, "Close valid FD succeeds");
+ ok(close(infd[0]) == 0, "Close valid FD succeeds");
+
+ ok(lttng_poll_wait(NULL, -1) == -1, "lttng_poll_wait call with invalid input returns error");
+
+ ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
+ ok(lttng_poll_wait(&poll_events, -1) == -1, "lttng_poll_wait call with invalid input returns error");
+ ok(lttng_poll_add(&poll_events, hupfd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
+ ok(lttng_write(infd[1], &tbuf, 1) == 1, "Write to pipe succeeds");
+ ok(lttng_poll_wait(&poll_events, -1) == 1, "Wakes up on one event");
+ ok(lttng_poll_del(&poll_events, hupfd[0]) == 0, "Removing valid FD succeeds");
+ ok(close(hupfd[0]) == 0, "Close valid FD succeeds");
+ ok(close(infd[1]) == 0, "Close valid FD succeeds");
+ lttng_poll_clean(&poll_events);
+
+ ok(waitpid(cpid, &wstatus, 0) == cpid, "waitpid returns child's pid");
+ ok(WIFEXITED(wstatus) == 1, "Child process exited");
+ ok(WEXITSTATUS(wstatus) == (EXIT_SUCCESS & 0xff), "EXIT_SUCCESS is %i while exit status is %i\n", EXIT_SUCCESS, WEXITSTATUS(wstatus));
+ }
+
+}
+
+void test_func_def(void)
+{
+#ifdef LTTNG_POLL_GETFD
+#define PASS_GETFD 1
+#else
+#define PASS_GETFD 0
+#endif
+
+#ifdef LTTNG_POLL_GETEV
+#define PASS_GETEV 1
+#else
+#define PASS_GETEV 0
+#endif
+
+#ifdef LTTNG_POLL_GETSZ
+#define PASS_GETSZ 1
+#else
+#define PASS_GETSZ 0
+#endif
+
+#ifdef LTTNG_POLL_GET_PREV_FD
+#define PASS_GET_PREV_FD 1
+#else
+#define PASS_GET_PREV_FD 0
+#endif
+
+ ok(lttng_poll_reset == lttng_poll_reset, "lttng_poll_reset is defined");
+ ok(lttng_poll_init == lttng_poll_init , "lttng_poll_reset is defined");
+ ok(PASS_GETFD, "GETFD is defined");
+ ok(PASS_GETEV, "GETEV is defined");
+ ok(PASS_GETSZ, "GETSZ is defined");
+ ok(PASS_GET_PREV_FD, "GET_PREV_FD is defined");
+
+}
+
+int main(void)
+{
+ plan_tests(NUM_TESTS);
+#ifdef HAVE_EPOLL
+ test_epoll_compat();
+#endif
+ test_func_def();
+ test_alloc();
+ test_add_del();
+ test_mod_wait();
+ return exit_status();
+}
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
2019-04-01 17:59 ` [PATCH lttng-tools v4 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
` (7 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Modify LTTNG_POLL_GETNB to provide compatibility with the epoll flavor.
Since it is only used after a lttng_poll_wait call with no modification
(add, del, mod) between, this change does not modify the behaviour in
its current usage while ensuring similar API behavior between
compatibility layer implementations.
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/common/compat/poll.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index fde54ddb..5eb7ff9c 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -315,10 +315,12 @@ static inline int __lttng_poll_get_prev_fd(struct lttng_poll_event *events,
/*
* For the following calls, consider 'e' to be a lttng_poll_event pointer and i
* being the index of the events array.
+ * LTTNG_POLL_GETNB is always used after lttng_poll_wait, thus we can use the
+ * current list for test compatibility purposes.
*/
#define LTTNG_POLL_GETFD(e, i) LTTNG_REF(e)->wait.events[i].fd
#define LTTNG_POLL_GETEV(e, i) LTTNG_REF(e)->wait.events[i].revents
-#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->wait.nb_fd
+#define LTTNG_POLL_GETNB(e) LTTNG_REF(e)->current.nb_fd
#define LTTNG_POLL_GETSZ(e) LTTNG_REF(e)->wait.events_size
#define LTTNG_POLL_GET_PREV_FD(e, i, nb_fd) \
__lttng_poll_get_prev_fd(LTTNG_REF(e), i, nb_fd)
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
` (2 preceding siblings ...)
2019-04-01 17:59 ` [PATCH lttng-tools v4 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 5/8] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
` (6 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/common/compat/compat-poll.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index b45b39dc..be655728 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2011 - David Goulet <david.goulet@polymtl.ca>
+ * Copyright (C) 2019 - Yannick Lamarre <ylamarre@efficios.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License, version 2 only,
@@ -204,10 +205,10 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
uint32_t req_events)
{
int i;
- bool fd_found = false;
struct compat_poll_event_array *current;
- if (events == NULL || events->current.events == NULL || fd < 0) {
+ if (events == NULL || events->current.nb_fd == 0 ||
+ events->current.events == NULL || fd < 0) {
ERR("Bad compat poll mod arguments");
goto error;
}
@@ -216,16 +217,16 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
for (i = 0; i < current->nb_fd; i++) {
if (current->events[i].fd == fd) {
- fd_found = true;
current->events[i].events = req_events;
events->need_update = 1;
break;
}
}
- if (!fd_found) {
- goto error;
- }
+ /*
+ * The epoll flavor doesn't flag modifying a non-included FD as an
+ * error.
+ */
return 0;
@@ -238,11 +239,12 @@ error:
*/
int compat_poll_del(struct lttng_poll_event *events, int fd)
{
- int new_size, i, count = 0, ret;
+ int i, count = 0, ret;
+ uint32_t new_size;
struct compat_poll_event_array *current;
- if (events == NULL || events->current.events == NULL || fd < 0) {
- ERR("Wrong arguments for poll del");
+ if (events == NULL || events->current.nb_fd == 0 ||
+ events->current.events == NULL || fd < 0) {
goto error;
}
@@ -257,13 +259,20 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
count++;
}
}
+
+ /* The fd was not in our set, return no error as with epoll. */
+ if (current->nb_fd == count) {
+ goto end;
+ }
+
/* No fd duplicate should be ever added into array. */
assert(current->nb_fd - 1 == count);
current->nb_fd = count;
/* Resize array if needed. */
new_size = 1U << utils_get_count_order_u32(current->nb_fd);
- if (new_size != current->alloc_size && new_size >= current->init_size) {
+ if (new_size != current->alloc_size && new_size >= current->init_size
+ && current->nb_fd != 0) {
ret = resize_poll_event(current, new_size);
if (ret < 0) {
goto error;
@@ -272,6 +281,7 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
events->need_update = 1;
+end:
return 0;
error:
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 5/8] Fix hang in thread_rotation when using compat-poll
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
` (3 preceding siblings ...)
2019-04-01 17:59 ` [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
` (5 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Add missing verification to prevent a blocking read on an empty fd.
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/bin/lttng-sessiond/rotation-thread.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index 6669372d..b86b1668 100644
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -974,6 +974,10 @@ void *thread_rotation(void *data)
int fd = LTTNG_POLL_GETFD(&thread.events, i);
uint32_t revents = LTTNG_POLL_GETEV(&thread.events, i);
+ if (!revents) {
+ /* No activity for this FD (poll implementation). */
+ continue;
+ }
DBG("[rotation-thread] Handling fd (%i) activity (%u)",
fd, revents);
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
` (4 preceding siblings ...)
2019-04-01 17:59 ` [PATCH lttng-tools v4 5/8] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 7/8] Clean code base from redundant verification Yannick Lamarre
` (4 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
This removes the need to verify for idle file descriptors and mitigates
risks of bug due to behaviour mismatch.
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
The assert in the for loop is removed.
src/common/compat/compat-poll.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
index be655728..5b752a28 100644
--- a/src/common/compat/compat-poll.c
+++ b/src/common/compat/compat-poll.c
@@ -293,7 +293,8 @@ error:
*/
int compat_poll_wait(struct lttng_poll_event *events, int timeout)
{
- int ret;
+ int ret, active_fd_count;
+ int idle_pfd_index = 0;
if (events == NULL || events->current.events == NULL) {
ERR("poll wait arguments error");
@@ -325,11 +326,39 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
goto error;
}
+ active_fd_count = ret;
+
/*
- * poll() should always iterate on all FDs since we handle the pollset in
- * user space and after poll returns, we have to try every fd for a match.
+ * Swap all active pollfd structs to the beginning of the
+ * array to emulate compat-epoll behaviour. This algorithm takes
+ * advantage of poll's returned value and the burst nature of active
+ * events on the file descriptors. The while loop guarantees that
+ * idle_pfd will always point to an idle fd.
*/
- return events->wait.nb_fd;
+ if (active_fd_count == events->wait.nb_fd) {
+ goto end;
+ }
+ while (idle_pfd_index < active_fd_count &&
+ events->wait.events[idle_pfd_index].revents != 0) {
+ idle_pfd_index++;
+ }
+
+ for (int i = idle_pfd_index + 1; idle_pfd_index < active_fd_count;
+ i++) {
+ struct pollfd swap_pfd;
+ struct pollfd *idle_pfd = &events->wait.events[idle_pfd_index];
+ struct pollfd *current_pfd = &events->wait.events[i];
+
+ if (ipfd->revents != 0) {
+ swap_pfd = *current_pfd;
+ *current_pfd = *idle_pfd;
+ *idle_pfd = swap_pfd;
+ idle_pfd_index++;
+ }
+ }
+
+end:
+ return ret;
error:
return -1;
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 7/8] Clean code base from redundant verification
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
` (5 preceding siblings ...)
2019-04-01 17:59 ` [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 8/8] Fix typo Yannick Lamarre
` (3 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Remove redundant verification for file descriptors with 0 revents in
code base.
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/bin/lttng-consumerd/health-consumerd.c | 5 -----
src/bin/lttng-relayd/health-relayd.c | 5 -----
src/bin/lttng-relayd/live.c | 10 ----------
src/bin/lttng-relayd/main.c | 16 ----------------
src/bin/lttng-sessiond/agent-thread.c | 5 -----
src/bin/lttng-sessiond/client.c | 5 -----
src/bin/lttng-sessiond/dispatch.c | 5 -----
src/bin/lttng-sessiond/health.c | 5 -----
src/bin/lttng-sessiond/ht-cleanup.c | 5 -----
src/bin/lttng-sessiond/manage-apps.c | 5 -----
src/bin/lttng-sessiond/manage-consumer.c | 10 ----------
src/bin/lttng-sessiond/manage-kernel.c | 5 -----
src/bin/lttng-sessiond/notification-thread.c | 3 ---
src/bin/lttng-sessiond/notify-apps.c | 5 -----
src/bin/lttng-sessiond/register.c | 5 -----
src/bin/lttng-sessiond/rotation-thread.c | 4 ----
src/common/consumer/consumer.c | 10 ----------
17 files changed, 108 deletions(-)
diff --git a/src/bin/lttng-consumerd/health-consumerd.c b/src/bin/lttng-consumerd/health-consumerd.c
index 1e2f31e4..7d60e54e 100644
--- a/src/bin/lttng-consumerd/health-consumerd.c
+++ b/src/bin/lttng-consumerd/health-consumerd.c
@@ -259,11 +259,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_health_quit_pipe(pollfd, revents);
if (ret) {
diff --git a/src/bin/lttng-relayd/health-relayd.c b/src/bin/lttng-relayd/health-relayd.c
index ba996621..b782c3e7 100644
--- a/src/bin/lttng-relayd/health-relayd.c
+++ b/src/bin/lttng-relayd/health-relayd.c
@@ -329,11 +329,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_health_quit_pipe(pollfd, revents);
if (ret) {
diff --git a/src/bin/lttng-relayd/live.c b/src/bin/lttng-relayd/live.c
index 9efab8a7..9503ba47 100644
--- a/src/bin/lttng-relayd/live.c
+++ b/src/bin/lttng-relayd/live.c
@@ -543,11 +543,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_thread_quit_pipe(pollfd, revents);
if (ret) {
@@ -2000,11 +1995,6 @@ restart:
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_thread_quit_pipe(pollfd, revents);
if (ret) {
diff --git a/src/bin/lttng-relayd/main.c b/src/bin/lttng-relayd/main.c
index e81bdeff..0ee27efc 100644
--- a/src/bin/lttng-relayd/main.c
+++ b/src/bin/lttng-relayd/main.c
@@ -858,14 +858,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /*
- * No activity for this FD (poll
- * implementation).
- */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_thread_quit_pipe(pollfd, revents);
if (ret) {
@@ -3813,14 +3805,6 @@ restart:
health_code_update();
- if (!revents) {
- /*
- * No activity for this FD (poll
- * implementation).
- */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
ret = check_thread_quit_pipe(pollfd, revents);
if (ret) {
diff --git a/src/bin/lttng-sessiond/agent-thread.c b/src/bin/lttng-sessiond/agent-thread.c
index f7be1ef7..575f6aee 100644
--- a/src/bin/lttng-sessiond/agent-thread.c
+++ b/src/bin/lttng-sessiond/agent-thread.c
@@ -404,11 +404,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
if (pollfd == quit_pipe_read_fd) {
goto exit;
diff --git a/src/bin/lttng-sessiond/client.c b/src/bin/lttng-sessiond/client.c
index a889529a..0be065ec 100644
--- a/src/bin/lttng-sessiond/client.c
+++ b/src/bin/lttng-sessiond/client.c
@@ -2174,11 +2174,6 @@ static void *thread_manage_clients(void *data)
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd == thread_quit_pipe_fd) {
err = 0;
goto exit;
diff --git a/src/bin/lttng-sessiond/dispatch.c b/src/bin/lttng-sessiond/dispatch.c
index 04b3954c..a637f23d 100644
--- a/src/bin/lttng-sessiond/dispatch.c
+++ b/src/bin/lttng-sessiond/dispatch.c
@@ -144,11 +144,6 @@ static void sanitize_wait_queue(struct ust_reg_wait_queue *wait_queue)
uint32_t revents = LTTNG_POLL_GETEV(&events, i);
int pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
cds_list_for_each_entry_safe(wait_node, tmp_wait_node,
&wait_queue->head, head) {
if (pollfd == wait_node->app->sock &&
diff --git a/src/bin/lttng-sessiond/health.c b/src/bin/lttng-sessiond/health.c
index 921b4526..5b4f5f8f 100644
--- a/src/bin/lttng-sessiond/health.c
+++ b/src/bin/lttng-sessiond/health.c
@@ -158,11 +158,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Event on the registration socket */
if (pollfd == sock) {
if (revents & LPOLLIN) {
diff --git a/src/bin/lttng-sessiond/ht-cleanup.c b/src/bin/lttng-sessiond/ht-cleanup.c
index 91c0544c..02244f0c 100644
--- a/src/bin/lttng-sessiond/ht-cleanup.c
+++ b/src/bin/lttng-sessiond/ht-cleanup.c
@@ -155,11 +155,6 @@ static void *thread_ht_cleanup(void *data)
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd != ht_cleanup_pipe[0]) {
continue;
}
diff --git a/src/bin/lttng-sessiond/manage-apps.c b/src/bin/lttng-sessiond/manage-apps.c
index f9ec356d..356c147a 100644
--- a/src/bin/lttng-sessiond/manage-apps.c
+++ b/src/bin/lttng-sessiond/manage-apps.c
@@ -123,11 +123,6 @@ static void *thread_application_management(void *data)
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd == quit_pipe_read_fd) {
err = 0;
goto exit;
diff --git a/src/bin/lttng-sessiond/manage-consumer.c b/src/bin/lttng-sessiond/manage-consumer.c
index b710c61f..fa802adc 100644
--- a/src/bin/lttng-sessiond/manage-consumer.c
+++ b/src/bin/lttng-sessiond/manage-consumer.c
@@ -134,11 +134,6 @@ void *thread_consumer_management(void *data)
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
if (pollfd == quit_pipe_read_fd) {
err = 0;
@@ -298,11 +293,6 @@ void *thread_consumer_management(void *data)
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/*
* Thread quit pipe has been triggered, flag that we should stop
* but continue the current loop to handle potential data from
diff --git a/src/bin/lttng-sessiond/manage-kernel.c b/src/bin/lttng-sessiond/manage-kernel.c
index 34887d7b..f656c9f5 100644
--- a/src/bin/lttng-sessiond/manage-kernel.c
+++ b/src/bin/lttng-sessiond/manage-kernel.c
@@ -267,11 +267,6 @@ static void *thread_kernel_management(void *data)
health_code_update();
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd == quit_pipe_read_fd) {
err = 0;
goto exit;
diff --git a/src/bin/lttng-sessiond/notification-thread.c b/src/bin/lttng-sessiond/notification-thread.c
index b42b282e..0a1fd557 100644
--- a/src/bin/lttng-sessiond/notification-thread.c
+++ b/src/bin/lttng-sessiond/notification-thread.c
@@ -562,9 +562,6 @@ void *thread_notification(void *data)
int fd = LTTNG_POLL_GETFD(&state.events, i);
uint32_t revents = LTTNG_POLL_GETEV(&state.events, i);
- if (!revents) {
- continue;
- }
DBG("[notification-thread] Handling fd (%i) activity (%u)", fd, revents);
if (fd == state.notification_channel_socket) {
diff --git a/src/bin/lttng-sessiond/notify-apps.c b/src/bin/lttng-sessiond/notify-apps.c
index bc7405c7..17f3cd0c 100644
--- a/src/bin/lttng-sessiond/notify-apps.c
+++ b/src/bin/lttng-sessiond/notify-apps.c
@@ -109,11 +109,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
if (pollfd == quit_pipe_read_fd) {
err = 0;
diff --git a/src/bin/lttng-sessiond/register.c b/src/bin/lttng-sessiond/register.c
index 6ce25ad0..e809834f 100644
--- a/src/bin/lttng-sessiond/register.c
+++ b/src/bin/lttng-sessiond/register.c
@@ -239,11 +239,6 @@ static void *thread_application_registration(void *data)
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
/* Thread quit pipe has been closed. Killing thread. */
if (pollfd == quit_pipe_read_fd) {
err = 0;
diff --git a/src/bin/lttng-sessiond/rotation-thread.c b/src/bin/lttng-sessiond/rotation-thread.c
index b86b1668..6669372d 100644
--- a/src/bin/lttng-sessiond/rotation-thread.c
+++ b/src/bin/lttng-sessiond/rotation-thread.c
@@ -974,10 +974,6 @@ void *thread_rotation(void *data)
int fd = LTTNG_POLL_GETFD(&thread.events, i);
uint32_t revents = LTTNG_POLL_GETEV(&thread.events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
DBG("[rotation-thread] Handling fd (%i) activity (%u)",
fd, revents);
diff --git a/src/common/consumer/consumer.c b/src/common/consumer/consumer.c
index 10273e66..c70d95a5 100644
--- a/src/common/consumer/consumer.c
+++ b/src/common/consumer/consumer.c
@@ -2396,11 +2396,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd == lttng_pipe_get_readfd(ctx->consumer_metadata_pipe)) {
if (revents & LPOLLIN) {
ssize_t pipe_len;
@@ -2990,11 +2985,6 @@ restart:
revents = LTTNG_POLL_GETEV(&events, i);
pollfd = LTTNG_POLL_GETFD(&events, i);
- if (!revents) {
- /* No activity for this FD (poll implementation). */
- continue;
- }
-
if (pollfd == ctx->consumer_channel_pipe[0]) {
if (revents & LPOLLIN) {
enum consumer_channel_action action;
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH lttng-tools v4 8/8] Fix typo
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
` (6 preceding siblings ...)
2019-04-01 17:59 ` [PATCH lttng-tools v4 7/8] Clean code base from redundant verification Yannick Lamarre
@ 2019-04-01 17:59 ` Yannick Lamarre
[not found] ` <20190401175925.8512-3-ylamarre@efficios.com>
` (2 subsequent siblings)
10 siblings, 0 replies; 11+ messages in thread
From: Yannick Lamarre @ 2019-04-01 17:59 UTC (permalink / raw)
To: lttng-dev; +Cc: jgalar
Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
---
src/common/compat/poll.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/common/compat/poll.h b/src/common/compat/poll.h
index 5eb7ff9c..d7020f36 100644
--- a/src/common/compat/poll.h
+++ b/src/common/compat/poll.h
@@ -358,7 +358,7 @@ extern int compat_poll_del(struct lttng_poll_event *events, int fd);
compat_poll_del(events, fd)
/*
- * Modify an fd's events in the epoll set.
+ * Modify an fd's events in the poll set.
*/
extern int compat_poll_mod(struct lttng_poll_event *events,
int fd, uint32_t req_events);
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer
[not found] ` <20190401175925.8512-3-ylamarre@efficios.com>
@ 2019-04-24 20:39 ` Jérémie Galarneau
0 siblings, 0 replies; 11+ messages in thread
From: Jérémie Galarneau @ 2019-04-24 20:39 UTC (permalink / raw)
To: Yannick Lamarre; +Cc: lttng-dev, jgalar
On Mon, Apr 01, 2019 at 01:59:19PM -0400, Yannick Lamarre wrote:
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> .gitignore | 1 +
> tests/unit/Makefile.am | 9 +-
> tests/unit/test_utils_compat_poll.c | 236 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 245 insertions(+), 1 deletion(-)
> create mode 100644 tests/unit/test_utils_compat_poll.c
>
> diff --git a/.gitignore b/.gitignore
> index 19276012..b5d2a55a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -73,6 +73,7 @@ TAGS
> /tests/unit/test_session
> /tests/unit/test_uri
> /tests/unit/test_ust_data
> +/tests/unit/test_utils_compat_poll
> /tests/unit/test_utils_parse_size_suffix
> /tests/unit/test_utils_parse_time_suffix
> /tests/unit/test_utils_expand_path
> diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
> index 5f485f00..bcda6a5c 100644
> --- a/tests/unit/Makefile.am
> +++ b/tests/unit/Makefile.am
> @@ -12,6 +12,7 @@ TESTS = test_kernel_data \
> test_utils_parse_size_suffix \
> test_utils_parse_time_suffix \
> test_utils_expand_path \
> + test_utils_compat_poll \
> test_string_utils \
> test_notification \
> ini_config/test_ini_config
> @@ -28,7 +29,8 @@ LIBLTTNG_CTL=$(top_builddir)/src/lib/lttng-ctl/liblttng-ctl.la
> # Define test programs
> noinst_PROGRAMS = test_uri test_session test_kernel_data \
> test_utils_parse_size_suffix test_utils_parse_time_suffix \
> - test_utils_expand_path test_string_utils test_notification
> + test_utils_expand_path test_utils_compat_poll \
> + test_string_utils test_notification
Use spaces to indent here.
>
> if HAVE_LIBLTTNG_UST_CTL
> noinst_PROGRAMS += test_ust_data
> @@ -145,6 +147,11 @@ test_utils_parse_size_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL
> test_utils_parse_time_suffix_SOURCES = test_utils_parse_time_suffix.c
> test_utils_parse_time_suffix_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON)
>
> +# compat_poll unit test
> +test_utils_compat_poll_SOURCES = test_utils_compat_poll.c
> +test_utils_compat_poll_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(DL_LIBS) \
> + $(top_builddir)/src/common/compat/libcompat.la $(LIBCOMMON)
> +
> # expand_path unit test
> test_utils_expand_path_SOURCES = test_utils_expand_path.c
> test_utils_expand_path_LDADD = $(LIBTAP) $(LIBHASHTABLE) $(LIBCOMMON) $(DL_LIBS)
> diff --git a/tests/unit/test_utils_compat_poll.c b/tests/unit/test_utils_compat_poll.c
> new file mode 100644
> index 00000000..f2c49254
> --- /dev/null
> +++ b/tests/unit/test_utils_compat_poll.c
> @@ -0,0 +1,236 @@
> +/*
> + * test_utils_compat_poll.c
> + *
> + * Unit tests for the compatibility layer of poll/epoll API.
> + *
> + * Copyright (C) 2019 Yannick Lamarre <ylamarre@efficios.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by as
> + * published by the Free Software Foundation; only version 2 of the License.
> + *
> + * 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 <assert.h>
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +
> +#include <tap/tap.h>
> +
> +#include <common/compat/poll.h>
> +#include <common/readwrite.h>
> +
> +/* Verification without trashing test order in the child process */
> +#define childok(e, test, ...) do { \
> + if (!(e)) { \
> + diag(test, ## __VA_ARGS__); \
> + _exit(EXIT_FAILURE); \
> + } \
> +} while(0)
> +
> +/* For error.h */
> +int lttng_opt_quiet = 1;
> +int lttng_opt_verbose;
> +int lttng_opt_mi;
> +
> +#ifdef HAVE_EPOLL
> +#define NUM_TESTS 46
> +#else
> +#define NUM_TESTS 45
> +#endif
> +
> +#ifdef HAVE_EPOLL
> +#if defined(HAVE_EPOLL_CREATE1) && defined(EPOLL_CLOEXEC)
> +#define CLOE_VALUE EPOLL_CLOEXEC
> +#else
> +#define CLOE_VALUE FD_CLOEXEC
> +#endif
> +
> +/* Non-zero 8-bits arbitrary value below 0x7f to ensure no sign extension
> + * used to verify that the value is properly propagated throught the pipe.
> + */
> +#define MAGIC_VALUE ((char)0x5A)
> +
> +void test_epoll_compat(void)
> +{
> + /*
> + * Type conversion present to disable warning of anonymous enum from
> + * compiler.
> + */
> + ok((int)LTTNG_CLOEXEC == (int)CLOE_VALUE, "epoll's CLOEXEC value");
Put a space between operators and operands (cast and value).
> +}
> +#endif
> +
> +void test_alloc(void)
> +{
> + struct lttng_poll_event poll_events;
> +
> + lttng_poll_init(&poll_events);
> +
> + /* Null pointer */
> + ok(lttng_poll_create(NULL, 1, 0) != 0, "Create over NULL pointer fails");
> + /* Size 0 */
> + ok(lttng_poll_create(&poll_events, 0, 0) != 0, "Create with size 0 fails");
> + /* without CLOEXEC */
> + ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
> + /*
> + * lttng_poll_event structure untested due to incompatibility across
> + * sublayers. lttng_poll_clean cannot be tested. There is no success
> + * criteria. Verify set's max size cases.
> + */
> + lttng_poll_clean(&poll_events);
> +}
> +
> +/* Tests stuff related to what would be handled with epoll_ctl. */
> +void test_add_del(void)
> +{
> + struct lttng_poll_event poll_events;
> +
> + lttng_poll_init(&poll_events);
> + ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
> + ok(lttng_poll_add(&poll_events, 1, LPOLLIN) != 0, "Adding to uninitialized structure fails");
> + ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
> +
> + lttng_poll_create(&poll_events, 1, 0);
> + ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set created empty");
> +
> + ok(lttng_poll_add(NULL, 1, LPOLLIN) != 0, "Adding to NULL set fails");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
> + ok(lttng_poll_add(&poll_events, -1, LPOLLIN) != 0, "Adding invalid FD fails");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Set still empty");
> +
> + ok(lttng_poll_add(&poll_events, 1, LPOLLIN) == 0, "Adding valid FD succeeds");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Nb of elements incremented");
> +
> + ok(lttng_poll_del(NULL, 1) != 0, "Removing from NULL set fails");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
> +
> + ok(lttng_poll_del(&poll_events, -1) != 0, "Removing from negative FD fails");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of FD in set unchanged");
> +
> + ok(lttng_poll_del(&poll_events, 2) == 0, "Removing invalid FD still succeeds");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 1, "Number of elements unchanged");
> +
> + ok(lttng_poll_del(&poll_events, 1) == 0, "Removing valid FD succeeds");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements decremented");
> +
> + ok(lttng_poll_del(&poll_events, 1) != 0, "Removing from empty set fails");
> + ok(LTTNG_POLL_GETNB(&poll_events) == 0, "Nb of elements unchanged");
> +
> + lttng_poll_clean(&poll_events);
> +}
> +
> +void test_mod_wait(void)
> +{
> + struct lttng_poll_event poll_events;
> + struct lttng_poll_event cpoll_events;
> +
Remove empty line here.
> + int hupfd[2];
> + int infd[2];
> + pid_t cpid;
> + char rbuf = 0, tbuf = MAGIC_VALUE;
> + int wstatus;
> +
> + lttng_poll_init(&poll_events);
> + lttng_poll_init(&cpoll_events);
> +
> + ok(pipe(hupfd) != -1);
> + ok(pipe(infd) != -1);
> +
> + cpid = fork();
> + if (cpid == 0) {
> + childok(lttng_poll_create(&cpoll_events, 1, 0) == 0, "Create valid poll set succeeds");
> + childok(lttng_poll_mod(NULL, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
> + childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
> + childok(lttng_poll_add(&cpoll_events, infd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
> + childok(lttng_poll_mod(&cpoll_events, -1, LPOLLIN) == -1, "lttng_poll_mod with invalid input returns an error");
> + childok(lttng_poll_mod(&cpoll_events, hupfd[0], LPOLLIN) == 0, "lttng_poll_mod on unincluded FD goes on");
> + childok(lttng_poll_mod(&cpoll_events, infd[0], LPOLLIN) == 0, "Modify event type succeeds");
> + childok(close(infd[1]) == 0, "Close valid FD succeeds");
> + childok(lttng_poll_wait(&cpoll_events, -1) == 1, "Wait on close times out");
> + childok(lttng_read(infd[0], &rbuf, 1) == 1, "Data is present in the pipe");
> + childok(rbuf == MAGIC_VALUE, "Received data is consistent with transmitted data");
> + childok(lttng_poll_del(&cpoll_events, infd[0]) == 0, "Removing valid FD succeeds");
> + childok(close(infd[0]) == 0, "Close valid FD succeeds");
> + childok(close(hupfd[0]) == 0, "Close valid FD succeeds");
> + childok(close(hupfd[1]) == 0, "Close valid FD succeeds");
> + lttng_poll_clean(&cpoll_events);
> + _exit(EXIT_SUCCESS);
> + } else {
> + ok(close(hupfd[1]) == 0, "Close valid FD succeeds");
> + ok(close(infd[0]) == 0, "Close valid FD succeeds");
> +
> + ok(lttng_poll_wait(NULL, -1) == -1, "lttng_poll_wait call with invalid input returns error");
> +
> + ok(lttng_poll_create(&poll_events, 1, 0) == 0, "Create valid poll set succeeds");
> + ok(lttng_poll_wait(&poll_events, -1) == -1, "lttng_poll_wait call with invalid input returns error");
> + ok(lttng_poll_add(&poll_events, hupfd[0], LPOLLHUP) == 0, "Add valid FD succeeds");
> + ok(lttng_write(infd[1], &tbuf, 1) == 1, "Write to pipe succeeds");
> + ok(lttng_poll_wait(&poll_events, -1) == 1, "Wakes up on one event");
> + ok(lttng_poll_del(&poll_events, hupfd[0]) == 0, "Removing valid FD succeeds");
> + ok(close(hupfd[0]) == 0, "Close valid FD succeeds");
> + ok(close(infd[1]) == 0, "Close valid FD succeeds");
> + lttng_poll_clean(&poll_events);
> +
> + ok(waitpid(cpid, &wstatus, 0) == cpid, "waitpid returns child's pid");
> + ok(WIFEXITED(wstatus) == 1, "Child process exited");
> + ok(WEXITSTATUS(wstatus) == (EXIT_SUCCESS & 0xff), "EXIT_SUCCESS is %i while exit status is %i\n", EXIT_SUCCESS, WEXITSTATUS(wstatus));
> + }
> +
> +}
> +
> +void test_func_def(void)
> +{
> +#ifdef LTTNG_POLL_GETFD
> +#define PASS_GETFD 1
> +#else
> +#define PASS_GETFD 0
> +#endif
> +
> +#ifdef LTTNG_POLL_GETEV
> +#define PASS_GETEV 1
> +#else
> +#define PASS_GETEV 0
> +#endif
> +
> +#ifdef LTTNG_POLL_GETSZ
> +#define PASS_GETSZ 1
> +#else
> +#define PASS_GETSZ 0
> +#endif
> +
> +#ifdef LTTNG_POLL_GET_PREV_FD
> +#define PASS_GET_PREV_FD 1
> +#else
> +#define PASS_GET_PREV_FD 0
> +#endif
> +
> + ok(lttng_poll_reset == lttng_poll_reset, "lttng_poll_reset is defined");
> + ok(lttng_poll_init == lttng_poll_init , "lttng_poll_reset is defined");
Replace 'reset' by 'init' in the test output.
> + ok(PASS_GETFD, "GETFD is defined");
> + ok(PASS_GETEV, "GETEV is defined");
> + ok(PASS_GETSZ, "GETSZ is defined");
> + ok(PASS_GET_PREV_FD, "GET_PREV_FD is defined");
> +
Remove empty line here.
Thanks!
Jérémie
> +}
> +
> +int main(void)
> +{
> + plan_tests(NUM_TESTS);
> +#ifdef HAVE_EPOLL
> + test_epoll_compat();
> +#endif
> + test_func_def();
> + test_alloc();
> + test_add_del();
> + test_mod_wait();
> + return exit_status();
> +}
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer
[not found] ` <20190401175925.8512-5-ylamarre@efficios.com>
@ 2019-04-24 21:02 ` Jérémie Galarneau
0 siblings, 0 replies; 11+ messages in thread
From: Jérémie Galarneau @ 2019-04-24 21:02 UTC (permalink / raw)
To: Yannick Lamarre; +Cc: lttng-dev, jgalar
On Mon, Apr 01, 2019 at 01:59:21PM -0400, Yannick Lamarre wrote:
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
> src/common/compat/compat-poll.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index b45b39dc..be655728 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -1,5 +1,6 @@
> /*
> * Copyright (C) 2011 - David Goulet <david.goulet@polymtl.ca>
> + * Copyright (C) 2019 - Yannick Lamarre <ylamarre@efficios.com>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License, version 2 only,
> @@ -204,10 +205,10 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
> uint32_t req_events)
> {
> int i;
> - bool fd_found = false;
> struct compat_poll_event_array *current;
>
> - if (events == NULL || events->current.events == NULL || fd < 0) {
> + if (events == NULL || events->current.nb_fd == 0 ||
> + events->current.events == NULL || fd < 0) {
> ERR("Bad compat poll mod arguments");
> goto error;
> }
> @@ -216,16 +217,16 @@ int compat_poll_mod(struct lttng_poll_event *events, int fd,
>
> for (i = 0; i < current->nb_fd; i++) {
> if (current->events[i].fd == fd) {
> - fd_found = true;
> current->events[i].events = req_events;
> events->need_update = 1;
> break;
> }
> }
>
> - if (!fd_found) {
> - goto error;
> - }
> + /*
Remove space after '/*'
Thanks!
Jérémie
> + * The epoll flavor doesn't flag modifying a non-included FD as an
> + * error.
> + */
>
> return 0;
>
> @@ -238,11 +239,12 @@ error:
> */
> int compat_poll_del(struct lttng_poll_event *events, int fd)
> {
> - int new_size, i, count = 0, ret;
> + int i, count = 0, ret;
> + uint32_t new_size;
> struct compat_poll_event_array *current;
>
> - if (events == NULL || events->current.events == NULL || fd < 0) {
> - ERR("Wrong arguments for poll del");
> + if (events == NULL || events->current.nb_fd == 0 ||
> + events->current.events == NULL || fd < 0) {
> goto error;
> }
>
> @@ -257,13 +259,20 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
> count++;
> }
> }
> +
> + /* The fd was not in our set, return no error as with epoll. */
> + if (current->nb_fd == count) {
> + goto end;
> + }
> +
> /* No fd duplicate should be ever added into array. */
> assert(current->nb_fd - 1 == count);
> current->nb_fd = count;
>
> /* Resize array if needed. */
> new_size = 1U << utils_get_count_order_u32(current->nb_fd);
> - if (new_size != current->alloc_size && new_size >= current->init_size) {
> + if (new_size != current->alloc_size && new_size >= current->init_size
> + && current->nb_fd != 0) {
> ret = resize_poll_event(current, new_size);
> if (ret < 0) {
> goto error;
> @@ -272,6 +281,7 @@ int compat_poll_del(struct lttng_poll_event *events, int fd)
>
> events->need_update = 1;
>
> +end:
> return 0;
>
> error:
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll
[not found] ` <20190401175925.8512-7-ylamarre@efficios.com>
@ 2019-04-24 21:08 ` Jérémie Galarneau
0 siblings, 0 replies; 11+ messages in thread
From: Jérémie Galarneau @ 2019-04-24 21:08 UTC (permalink / raw)
To: Yannick Lamarre; +Cc: lttng-dev, jgalar
On Mon, Apr 01, 2019 at 01:59:23PM -0400, Yannick Lamarre wrote:
> This removes the need to verify for idle file descriptors and mitigates
> risks of bug due to behaviour mismatch.
>
> Signed-off-by: Yannick Lamarre <ylamarre@efficios.com>
> ---
>
> The assert in the for loop is removed.
>
> src/common/compat/compat-poll.c | 37 +++++++++++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/src/common/compat/compat-poll.c b/src/common/compat/compat-poll.c
> index be655728..5b752a28 100644
> --- a/src/common/compat/compat-poll.c
> +++ b/src/common/compat/compat-poll.c
> @@ -293,7 +293,8 @@ error:
> */
> int compat_poll_wait(struct lttng_poll_event *events, int timeout)
> {
> - int ret;
> + int ret, active_fd_count;
> + int idle_pfd_index = 0;
>
> if (events == NULL || events->current.events == NULL) {
> ERR("poll wait arguments error");
> @@ -325,11 +326,39 @@ int compat_poll_wait(struct lttng_poll_event *events, int timeout)
> goto error;
> }
>
> + active_fd_count = ret;
> +
> /*
> - * poll() should always iterate on all FDs since we handle the pollset in
> - * user space and after poll returns, we have to try every fd for a match.
> + * Swap all active pollfd structs to the beginning of the
> + * array to emulate compat-epoll behaviour. This algorithm takes
> + * advantage of poll's returned value and the burst nature of active
> + * events on the file descriptors. The while loop guarantees that
> + * idle_pfd will always point to an idle fd.
> */
> - return events->wait.nb_fd;
> + if (active_fd_count == events->wait.nb_fd) {
> + goto end;
> + }
> + while (idle_pfd_index < active_fd_count &&
> + events->wait.events[idle_pfd_index].revents != 0) {
> + idle_pfd_index++;
> + }
> +
> + for (int i = idle_pfd_index + 1; idle_pfd_index < active_fd_count;
> + i++) {
Declare 'i' at the beginning of the function.
Thanks!
Jérémie
> + struct pollfd swap_pfd;
> + struct pollfd *idle_pfd = &events->wait.events[idle_pfd_index];
> + struct pollfd *current_pfd = &events->wait.events[i];
> +
> + if (ipfd->revents != 0) {
> + swap_pfd = *current_pfd;
> + *current_pfd = *idle_pfd;
> + *idle_pfd = swap_pfd;
> + idle_pfd_index++;
> + }
> + }
> +
> +end:
> + return ret;
>
> error:
> return -1;
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-24 21:08 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190401175925.8512-1-ylamarre@efficios.com>
2019-04-01 17:59 ` [PATCH lttng-tools v4 1/8] Fix: lttng_poll_mod calls compat_(e)poll_add Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 3/8] Change LTTNG_POLL_GETNB behaviour for poll flavor Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 5/8] Fix hang in thread_rotation when using compat-poll Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 7/8] Clean code base from redundant verification Yannick Lamarre
2019-04-01 17:59 ` [PATCH lttng-tools v4 8/8] Fix typo Yannick Lamarre
[not found] ` <20190401175925.8512-3-ylamarre@efficios.com>
2019-04-24 20:39 ` [PATCH lttng-tools v4 2/8] Add Unit test to poll compatibility layer Jérémie Galarneau
[not found] ` <20190401175925.8512-5-ylamarre@efficios.com>
2019-04-24 21:02 ` [PATCH lttng-tools v4 4/8] Adapt poll layer behaviour to match the epoll layer Jérémie Galarneau
[not found] ` <20190401175925.8512-7-ylamarre@efficios.com>
2019-04-24 21:08 ` [PATCH lttng-tools v4 6/8] Change lttng_poll_wait behaviour of compat-poll to match compat-epoll Jérémie Galarneau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).