Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Link Event Damping] Add tracker to track the selectable timers used by link event damper #1323

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions syncd/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ libSyncd_a_SOURCES = \
SaiObj.cpp \
SaiSwitch.cpp \
SaiSwitchInterface.cpp \
SelectablesTracker.cpp \
ServiceMethodTable.cpp \
SingleReiniter.cpp \
SwitchNotifications.cpp \
Expand Down
103 changes: 103 additions & 0 deletions syncd/SelectablesTracker.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
#include "SelectablesTracker.h"

#include "swss/logger.h"

using namespace syncd;

bool SelectablesTracker::addSelectableToTracker(
_In_ swss::Selectable *selectable,
_In_ SelectableEventHandler *eventHandler)
{
SWSS_LOG_ENTER();

if (selectable == nullptr)
{
SWSS_LOG_ERROR("Invalid Selectable:Selectable is NULL.");

return false;
}

int fd = selectable->getFd();
if (eventHandler == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the check before line 20

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

althoug he wanted to log fd number in error message, anyway please add empty line before if()

{
SWSS_LOG_ERROR("Event handler for Selectable with fd: %d is NULL.", fd);

return false;
}

if (m_selectableFdToEventHandlerMap.count(fd) != 0)
{
SWSS_LOG_ERROR("Selectable with fd %d is already in use.", fd);

return false;
}

SWSS_LOG_INFO("Adding the Selectable with fd: %d.", fd);
m_selectableFdToEventHandlerMap[fd] = eventHandler;

return true;
}

bool SelectablesTracker::removeSelectableFromTracker(
_In_ swss::Selectable *selectable)
{
SWSS_LOG_ENTER();

if (selectable == nullptr)
{
SWSS_LOG_ERROR("Invalid Selectable:Selectable is NULL.");

return false;
}

int fd = selectable->getFd();

SWSS_LOG_INFO("Removing the Selectable with fd: %d.", fd);
if (m_selectableFdToEventHandlerMap.erase(fd) == 0)
{
SWSS_LOG_ERROR("Selectable with fd %d is not present in the map!", fd);

return false;
}

return true;
}

bool SelectablesTracker::selectableIsTracked(
_In_ swss::Selectable *selectable)
{
SWSS_LOG_ENTER();

if ((selectable == nullptr) ||
(m_selectableFdToEventHandlerMap.count(selectable->getFd()) == 0))
{
return false;
}

return true;
}

SelectableEventHandler *SelectablesTracker::getEventHandlerForSelectable(
_In_ swss::Selectable *selectable)
{
SWSS_LOG_ENTER();

if (selectable == nullptr)
{
SWSS_LOG_ERROR("Invalid Selectable:Selectable is NULL.");

return nullptr;
}

int fd = selectable->getFd();
auto it = m_selectableFdToEventHandlerMap.find(fd);

if (it == m_selectableFdToEventHandlerMap.end())
{
SWSS_LOG_ERROR("Selectable with fd %d is not present in the map!", fd);

return nullptr;
}

return it->second;
}
54 changes: 54 additions & 0 deletions syncd/SelectablesTracker.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#pragma once

#include <unordered_map>

#include "SelectableEventHandler.h"
#include "swss/sal.h"
#include "swss/selectable.h"
#include "swss/selectabletimer.h"

namespace syncd
{
// This class keeps track of Selectable being used by an entity and their
// corresponding EventHandler objects.
class SelectablesTracker
{
private:

// Not copyable or movable.
Ashish1805 marked this conversation as resolved.
Show resolved Hide resolved
SelectablesTracker(const SelectablesTracker &) = delete;
SelectablesTracker(SelectablesTracker &&) = delete;
SelectablesTracker &operator=(const SelectablesTracker &) = delete;
SelectablesTracker &operator=(SelectablesTracker &&) = delete;

public:

SelectablesTracker() = default;

virtual ~SelectablesTracker() = default;

// Adds the mapping of Selectable and it's corresponding EventHandler object.
virtual bool addSelectableToTracker(
_In_ swss::Selectable *selectable,
_In_ SelectableEventHandler *eventHandler);

// Removes a Selectable from the map.
virtual bool removeSelectableFromTracker(
_In_ swss::Selectable *selectable);

// Checks if Selectable is present in the tracker map.
virtual bool selectableIsTracked(
_In_ swss::Selectable *selectable);

// Gets the EventHandler for a Selectable.
virtual SelectableEventHandler *getEventHandlerForSelectable(
_In_ swss::Selectable *selectable);

private:

using SelectableFdToEventHandlerMap = std::unordered_map<int, SelectableEventHandler *>;
Comment on lines +31 to +49
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use shared_ptr instead of pointers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like w should be using shared_ptr here. Do you use any benefit of using shared_ptr?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, benefit is to not lose trakc of pointers and prevent memory leak, please use shared_ptr, as is used in entire syncd project


SelectableFdToEventHandlerMap m_selectableFdToEventHandlerMap;
};

} // namespace syncd
1 change: 1 addition & 0 deletions tests/aspell.en.pws
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ config
const
CONST
consts
copyable
counterName
countOnly
cout
Expand Down
1 change: 1 addition & 0 deletions unittest/syncd/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ tests_SOURCES = main.cpp \
TestNotificationHandler.cpp \
TestMdioIpcServer.cpp \
TestPortStateChangeHandler.cpp \
TestSelectablesTracker.cpp \
TestVendorSai.cpp

tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON)
Expand Down
23 changes: 23 additions & 0 deletions unittest/syncd/MockSelectablesTracker.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#pragma once

#include "SelectablesTracker.h"
#include "gmock/gmock.h"

namespace syncd
{

class MockSelectablesTracker : public SelectablesTracker
{
public:

MockSelectablesTracker() : SelectablesTracker() {}

~MockSelectablesTracker() override {}

MOCK_METHOD2(addSelectableToTracker, bool(swss::Selectable *selectable,
SelectableEventHandler *eventHandler) override);

MOCK_METHOD1(removeSelectableFromTracker,
bool(swss::Selectable *selectable) override);
};
} // namespace syncd
161 changes: 161 additions & 0 deletions unittest/syncd/TestSelectablesTracker.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
#include <gtest/gtest.h>

#include "SelectablesTracker.h"

using namespace syncd;

class SelectableEventHandlerTestHelper : public SelectableEventHandler
{
public:

SelectableEventHandlerTestHelper() {}
~SelectableEventHandlerTestHelper() {}

void handleSelectableEvent() {}
};

class SelectablesTrackerTest : public ::testing::Test
{
public:

SelectablesTrackerTest() {}

SelectablesTracker m_selectablesTracker_;
};

TEST_F(SelectablesTrackerTest, AddSelectableFailsIfSelectableNull)
{
SelectableEventHandlerTestHelper eventHandler;

EXPECT_FALSE(m_selectablesTracker_.addSelectableToTracker(
/*selectable=*/nullptr, (SelectableEventHandler *)&eventHandler));
}

TEST_F(SelectablesTrackerTest, AddSelectableFailsIfEventHandlerNull)
{
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});

EXPECT_FALSE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, /*eventHandler=*/nullptr));
}

TEST_F(SelectablesTrackerTest, AddSelectableSucceeds)
{
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
SelectableEventHandlerTestHelper eventHandler;

EXPECT_TRUE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler));
}

TEST_F(SelectablesTrackerTest, AddSelectableFailsIfSelectableExists)
{
// Add a selectable timer to tracker.
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
SelectableEventHandlerTestHelper eventHandler;

EXPECT_TRUE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler));

// Adding the same selectable timer to tracker should fail.
SelectableEventHandlerTestHelper eventHandler2;
EXPECT_FALSE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler2));
}

TEST_F(SelectablesTrackerTest, RemoveSelectableFailsIfSelectableNull)
{
EXPECT_FALSE(
m_selectablesTracker_.removeSelectableFromTracker(/*selectable=*/nullptr));
}

TEST_F(SelectablesTrackerTest, RemoveSelectableFailsIfSelectableNotExist)
{
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});

EXPECT_FALSE(m_selectablesTracker_.removeSelectableFromTracker(
(swss::Selectable *)&timer));
}

TEST_F(SelectablesTrackerTest, RemoveSelectableSucceeds)
{
// Add a selectable timer.
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
SelectableEventHandlerTestHelper eventHandler;

EXPECT_TRUE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler));

// Remove the selectable timer just added in tracker.
EXPECT_TRUE(m_selectablesTracker_.removeSelectableFromTracker(
(swss::Selectable *)&timer));

// Removing the selectable timer again should fail.
EXPECT_FALSE(m_selectablesTracker_.removeSelectableFromTracker(
(swss::Selectable *)&timer));
}

TEST_F(SelectablesTrackerTest, NullptrSelectableIsNotTracked)
{
EXPECT_FALSE(
m_selectablesTracker_.selectableIsTracked(/*selectable=*/nullptr));
}

TEST_F(SelectablesTrackerTest, SelectableIsNotTracked)
{
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
EXPECT_FALSE(
m_selectablesTracker_.selectableIsTracked((swss::Selectable *)&timer));
}

TEST_F(SelectablesTrackerTest, SelectableIsTracked)
{
// Add the Selectable in the tracker.
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
SelectableEventHandlerTestHelper eventHandler;

EXPECT_TRUE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler));

// Verify the Selectable in the tracker.
EXPECT_TRUE(
m_selectablesTracker_.selectableIsTracked((swss::Selectable *)&timer));

// Remove the Selectable from tracker.
EXPECT_TRUE(m_selectablesTracker_.removeSelectableFromTracker(
(swss::Selectable *)&timer));

// Check the Selectable in tracker again, this time it should fail.
EXPECT_FALSE(
m_selectablesTracker_.selectableIsTracked((swss::Selectable *)&timer));
}

TEST_F(SelectablesTrackerTest, GetEventHandlerFailsIfSelectableNull)
{
EXPECT_EQ(
m_selectablesTracker_.getEventHandlerForSelectable(/*selectable=*/nullptr),
nullptr);
}

TEST_F(SelectablesTrackerTest, GetEventHandlerFailsIfSelectableNotExist)
{
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
EXPECT_EQ(m_selectablesTracker_.getEventHandlerForSelectable(
(swss::Selectable *)&timer),
nullptr);
}

TEST_F(SelectablesTrackerTest, GetEventHandlerSucceeds)
{
// Add a selectable timer.
swss::SelectableTimer timer(/*interval=*/{.tv_sec = 10, .tv_nsec = 20});
SelectableEventHandlerTestHelper eventHandler;

EXPECT_TRUE(m_selectablesTracker_.addSelectableToTracker(
(swss::Selectable *)&timer, (SelectableEventHandler *)&eventHandler));

// Get the event handler for the selectable timer.
EXPECT_EQ(m_selectablesTracker_.getEventHandlerForSelectable(
(swss::Selectable *)&timer),
(SelectableEventHandler *)&eventHandler);
}