From 1e21dbf7fac977835180cac3ee69aa103ff3ec59 Mon Sep 17 00:00:00 2001 From: Dante Su Date: Thu, 28 Apr 2022 09:11:52 +0000 Subject: [PATCH 1/3] selectabletimer: add mutex to start() and stop() Add mutex protection to avoid having the timer expiration reset due to subsequent calls of start() Avoid having the timer expiration reset due to subsequent calls of start() Add mutex protection to start() and stop() Signed-off-by: Dante Su --- common/selectabletimer.cpp | 35 +++++++++++++++++++++++++++-------- common/selectabletimer.h | 3 +++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/common/selectabletimer.cpp b/common/selectabletimer.cpp index 6f6bc0171..a10747ef4 100644 --- a/common/selectabletimer.cpp +++ b/common/selectabletimer.cpp @@ -21,6 +21,7 @@ SelectableTimer::SelectableTimer(const timespec& interval, int pri) SWSS_LOG_THROW("failed to create timerfd, errno: %s", strerror(errno)); } setInterval(interval); + m_alive = false; } SelectableTimer::~SelectableTimer() @@ -36,22 +37,40 @@ SelectableTimer::~SelectableTimer() void SelectableTimer::start() { - // Set the timer interval and the timer is automatically started - int rc = timerfd_settime(m_tfd, 0, &m_interval, NULL); - if (rc == -1) + m_mutex.lock(); + if (!m_alive) { - SWSS_LOG_THROW("failed to set timerfd, errno: %s", strerror(errno)); + // Set the timer interval and the timer is automatically started + int rc = timerfd_settime(m_tfd, 0, &m_interval, NULL); + if (rc == -1) + { + SWSS_LOG_THROW("failed to set timerfd, errno: %s", strerror(errno)); + } + else + { + m_alive = true; + } } + m_mutex.unlock(); } void SelectableTimer::stop() { - // Set the timer interval and the timer is automatically started - int rc = timerfd_settime(m_tfd, 0, &m_zero, NULL); - if (rc == -1) + m_mutex.lock(); + if (m_alive) { - SWSS_LOG_THROW("failed to set timerfd to zero, errno: %s", strerror(errno)); + // Set the timer interval and the timer is automatically started + int rc = timerfd_settime(m_tfd, 0, &m_zero, NULL); + if (rc == -1) + { + SWSS_LOG_THROW("failed to set timerfd to zero, errno: %s", strerror(errno)); + } + else + { + m_alive = false; + } } + m_mutex.unlock(); } void SelectableTimer::reset() diff --git a/common/selectabletimer.h b/common/selectabletimer.h index f7b416b2e..9a7f3c952 100644 --- a/common/selectabletimer.h +++ b/common/selectabletimer.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "selectable.h" @@ -22,6 +23,8 @@ class SelectableTimer : public Selectable uint64_t readData() override; private: + std::mutex m_mutex; + bool m_alive; int m_tfd; itimerspec m_interval; itimerspec m_zero; From 67fb5d1bcd8c7f52c6a834819bf86dd31510728a Mon Sep 17 00:00:00 2001 From: Dante Su Date: Mon, 9 May 2022 03:24:11 +0000 Subject: [PATCH 2/3] add timer_ut and use 'm_running' instead of 'm_alive' Signed-off-by: Dante Su --- common/selectabletimer.cpp | 10 +++---- common/selectabletimer.h | 2 +- tests/Makefile.am | 1 + tests/redis_ut.cpp | 36 ---------------------- tests/timer_ut.cpp | 61 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 42 deletions(-) create mode 100644 tests/timer_ut.cpp diff --git a/common/selectabletimer.cpp b/common/selectabletimer.cpp index a10747ef4..087c40621 100644 --- a/common/selectabletimer.cpp +++ b/common/selectabletimer.cpp @@ -21,7 +21,7 @@ SelectableTimer::SelectableTimer(const timespec& interval, int pri) SWSS_LOG_THROW("failed to create timerfd, errno: %s", strerror(errno)); } setInterval(interval); - m_alive = false; + m_running = false; } SelectableTimer::~SelectableTimer() @@ -38,7 +38,7 @@ SelectableTimer::~SelectableTimer() void SelectableTimer::start() { m_mutex.lock(); - if (!m_alive) + if (!m_running) { // Set the timer interval and the timer is automatically started int rc = timerfd_settime(m_tfd, 0, &m_interval, NULL); @@ -48,7 +48,7 @@ void SelectableTimer::start() } else { - m_alive = true; + m_running = true; } } m_mutex.unlock(); @@ -57,7 +57,7 @@ void SelectableTimer::start() void SelectableTimer::stop() { m_mutex.lock(); - if (m_alive) + if (m_running) { // Set the timer interval and the timer is automatically started int rc = timerfd_settime(m_tfd, 0, &m_zero, NULL); @@ -67,7 +67,7 @@ void SelectableTimer::stop() } else { - m_alive = false; + m_running = false; } } m_mutex.unlock(); diff --git a/common/selectabletimer.h b/common/selectabletimer.h index 9a7f3c952..2572881f1 100644 --- a/common/selectabletimer.h +++ b/common/selectabletimer.h @@ -24,7 +24,7 @@ class SelectableTimer : public Selectable private: std::mutex m_mutex; - bool m_alive; + bool m_running; int m_tfd; itimerspec m_interval; itimerspec m_zero; diff --git a/tests/Makefile.am b/tests/Makefile.am index a84a6df95..19e04250c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ tests_SOURCES = redis_ut.cpp \ boolean_ut.cpp \ status_code_util_test.cpp \ saiaclschema_ut.cpp \ + timer_ut.cpp \ main.cpp tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index dd6292f0c..f33cf52ab 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -653,42 +653,6 @@ TEST(DBConnector, selectableevent) EXPECT_EQ(value, 2); } -TEST(DBConnector, selectabletimer) -{ - timespec interval = { .tv_sec = 1, .tv_nsec = 0 }; - SelectableTimer timer(interval); - - Select s; - s.addSelectable(&timer); - Selectable *sel; - int result; - - // Wait a non started timer - result = s.select(&sel, 2000); - ASSERT_EQ(result, Select::TIMEOUT); - - // Wait long enough so we got timer notification first - timer.start(); - result = s.select(&sel, 2000); - ASSERT_EQ(result, Select::OBJECT); - ASSERT_EQ(sel, &timer); - - // Wait short so we got select timeout first - result = s.select(&sel, 10); - ASSERT_EQ(result, Select::TIMEOUT); - - // Wait long enough so we got timer notification first - result = s.select(&sel, 10000); - ASSERT_EQ(result, Select::OBJECT); - ASSERT_EQ(sel, &timer); - - // Reset and wait long enough so we got timer notification first - timer.reset(); - result = s.select(&sel, 10000); - ASSERT_EQ(result, Select::OBJECT); - ASSERT_EQ(sel, &timer); -} - TEST(Table, basic) { TableBasicTest("TABLE_UT_TEST", true); diff --git a/tests/timer_ut.cpp b/tests/timer_ut.cpp new file mode 100644 index 000000000..92286d0eb --- /dev/null +++ b/tests/timer_ut.cpp @@ -0,0 +1,61 @@ +#include + +#include "common/dbconnector.h" +#include "common/consumertable.h" +#include "common/notificationconsumer.h" +#include "common/select.h" +#include "common/selectableevent.h" +#include "common/selectabletimer.h" +#include "common/subscriberstatetable.h" +#include "common/netmsg.h" +#include "common/netlink.h" +#include "gtest/gtest.h" + +using namespace std; +using namespace swss; + +TEST(TIMER, selectabletimer) +{ + timespec interval = { .tv_sec = 1, .tv_nsec = 0 }; + SelectableTimer timer(interval); + + Select s; + s.addSelectable(&timer); + Selectable *sel; + int result; + + // Wait a non started timer + result = s.select(&sel, 2000); + ASSERT_EQ(result, Select::TIMEOUT); + + // Wait long enough so we got timer notification first + timer.start(); + result = s.select(&sel, 2000); + ASSERT_EQ(result, Select::OBJECT); + ASSERT_EQ(sel, &timer); + + // Wait short so we got select timeout first + result = s.select(&sel, 10); + ASSERT_EQ(result, Select::TIMEOUT); + + // Wait long enough so we got timer notification first + result = s.select(&sel, 10000); + ASSERT_EQ(result, Select::OBJECT); + ASSERT_EQ(sel, &timer); + + // Reset and wait long enough so we got timer notification first + timer.reset(); + result = s.select(&sel, 10000); + ASSERT_EQ(result, Select::OBJECT); + ASSERT_EQ(sel, &timer); + + // Check if the timer gets reset by subsequent timer.start() + for (int t = 0; t < 2000; ++t) + { + timer.start(); + usleep(1000); + } + result = s.select(&sel, 1); + ASSERT_EQ(result, Select::OBJECT); + ASSERT_EQ(sel, &timer); +} From 11a5f685b2f4b2129a29bf08bd1cef9098f78c13 Mon Sep 17 00:00:00 2001 From: Dante Su Date: Mon, 9 May 2022 03:32:36 +0000 Subject: [PATCH 3/3] remove unused headers Signed-off-by: Dante Su --- tests/timer_ut.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/timer_ut.cpp b/tests/timer_ut.cpp index 92286d0eb..2cb6db6ec 100644 --- a/tests/timer_ut.cpp +++ b/tests/timer_ut.cpp @@ -1,14 +1,5 @@ -#include - -#include "common/dbconnector.h" -#include "common/consumertable.h" -#include "common/notificationconsumer.h" #include "common/select.h" -#include "common/selectableevent.h" #include "common/selectabletimer.h" -#include "common/subscriberstatetable.h" -#include "common/netmsg.h" -#include "common/netlink.h" #include "gtest/gtest.h" using namespace std;