[Link Event Damping] Add tracker to track the selectable timers used by link event damper by Ashish1805 · Pull Request #1323 · sonic-net/sonic-sairedis

Ashish1805 · 2023-11-17T22:35:54Z

Link event damper running on each port will have its own SelectableTimer. This class helps create a mapping of SelectableTimer and object that created the SelectableTimer so that when timer is fired, proper EventHandler object can be invoked.

Ashish1805 · 2023-11-18T00:37:04Z

Hi @kcudnik CodeQL / Analyze (cpp) (pull_request_target) check is failing.

/usr/bin/ld: ../../syncd/libSyncd.a(libSyncd_a-PortStateChangeHandler.o): in function `syncd::PortStateChangeHandler::PortStateChangeHandler(std::shared_ptr<swss::SelectableEvent>)':
/home/runner/work/sonic-sairedis/sonic-sairedis/syncd/PortStateChangeHandler.cpp:21: undefined reference to `syncd::PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE'
collect2: error: ld returned 1 exit status

PORT_STATE_CHANGE_QUEUE_SIZE is defined as static and constexpr class data member (https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/PortStateChangeHandler.h#L43). I see sairedis repo uses c++14, so it is necessary to also provide definition for this static data member in .cc file. Like:

// Defined in PortStateChangeHandler.cc
constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

PortStateChangeHandler.cpp was added in #1310 PR and CodeQL / Analyze (cpp) (pull_request_target) check passed in that PR: https://github.com/sonic-net/sonic-sairedis/actions/runs/6885079249. Not sure why it didn't fail during CodeQL check of that PR.

I am not sure what is the protocol here: do we add fix PR for this or revert the culprit PR #1310 and merge it again with suggested changes?

kcudnik · 2023-11-18T08:36:35Z

Hi @kcudnik CodeQL / Analyze (cpp) (pull_request_target) check is failing.
/usr/bin/ld: ../../syncd/libSyncd.a(libSyncd_a-PortStateChangeHandler.o): in function `syncd::PortStateChangeHandler::PortStateChangeHandler(std::shared_ptr<swss::SelectableEvent>)':
/home/runner/work/sonic-sairedis/sonic-sairedis/syncd/PortStateChangeHandler.cpp:21: undefined reference to `syncd::PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE'
collect2: error: ld returned 1 exit status
PORT_STATE_CHANGE_QUEUE_SIZE is defined as static and constexpr class data member (https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/PortStateChangeHandler.h#L43). I see sairedis repo uses c++14, so it is necessary to also provide definition for this static data member in .cc file. Like:
// Defined in PortStateChangeHandler.cc
constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;
PortStateChangeHandler.cpp was added in #1310 PR and CodeQL / Analyze (cpp) (pull_request_target) check passed in that PR: https://github.com/sonic-net/sonic-sairedis/actions/runs/6885079249. Not sure why it didn't fail during CodeQL check of that PR.

I am not sure what is the protocol here: do we add fix PR for this or revert the culprit PR #1310 and merge it again with suggested changes?

SAME ISSUE CAUSES SWSS ERROR HERE: #1310 (comment)

please figure out why this is causing build error or revert the change !

syncd/SelectablesTracker.h

kcudnik · 2023-11-18T08:38:43Z

syncd/SelectablesTracker.h

+            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 *>;


use shared_ptr instead of pointers

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

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

kcudnik

address comments

kcudnik · 2023-11-18T08:41:30Z

change

static constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

to

constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

first keyword static requires declaration in cpp file since it's considered a field

link event damper. - Link event damper running on each port will have its own SelectableTimer. This class helps create a mapping of SelectableTimer and object that created the SelectableTimer so that when timer is fired, proper EventHandler object can be invoked. HLD: sonic-net/SONiC#1071

Ashish1805 · 2023-11-28T05:40:34Z

Adding @Junchao-Mellanox for review.

Junchao-Mellanox · 2023-11-28T09:35:31Z

syncd/SelectablesTracker.cpp

+    }
+
+    int fd = selectable->getFd();
+    if (eventHandler == nullptr)


move the check before line 20

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

DendroLabs · 2026-03-17T14:22:41Z

Hi @Ashish1805 and @kcudnik,

This PR has been inactive for nearly 2 years. I'm picking up the link event damping feature and plan to open a new PR that supersedes this one, addressing all outstanding review comments:

Move class members to private
Replace raw pointers with shared_ptr (consistent with syncd codebase)
Fix code ordering per @Junchao-Mellanox's feedback
Add constexpr out-of-line definition for PORT_STATE_CHANGE_QUEUE_SIZE (fixes CodeQL build)
Rebase onto current master

Will credit @Ashish1805 as original author. See also my comment on #1334 regarding the overall design clarification.

@Ashish1805

…king Supersedes PR sonic-net#1323 by @Ashish1805. Adds SelectablesTracker class that tracks Selectable objects and their corresponding EventHandler mappings, used by the link event damper feature. Changes from original PR addressing review feedback: - Move non-copyable/non-movable declarations to private section (@kcudnik) - Replace raw pointers with shared_ptr for EventHandler to prevent memory leaks, consistent with syncd project conventions (@kcudnik) - Reorder null checks: check eventHandler before accessing selectable fd (@Junchao-Mellanox) - Add empty line before if() for consistent formatting (@kcudnik) Co-Authored-By: Ashish Singh <ashish.singh@google.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@Ashish1805

…king Supersedes PR sonic-net#1323 by @Ashish1805. Adds SelectablesTracker class that tracks Selectable objects and their corresponding EventHandler mappings, used by the link event damper feature. Changes from original PR addressing review feedback: - Move non-copyable/non-movable declarations to private section (@kcudnik) - Replace raw pointers with shared_ptr for EventHandler to prevent memory leaks, consistent with syncd project conventions (@kcudnik) - Reorder null checks: check eventHandler before accessing selectable fd (@Junchao-Mellanox) - Add empty line before if() for consistent formatting (@kcudnik) Signed-off-by: DendroLabs <info@dendrolabs.com>

kcudnik reviewed Nov 18, 2023

View reviewed changes

syncd/SelectablesTracker.h Show resolved Hide resolved

kcudnik reviewed Nov 18, 2023

View reviewed changes

kcudnik requested changes Nov 18, 2023

View reviewed changes

Ashish1805 force-pushed the ledamping_seh branch from 3198dc1 to 38739fb Compare November 28, 2023 01:03

Ashish1805 force-pushed the ledamping_seh branch from 38739fb to 0fe378f Compare November 28, 2023 01:04

Junchao-Mellanox reviewed Nov 28, 2023

View reviewed changes

Ashish1805 mentioned this pull request Dec 7, 2023

Link event damping HLD sonic-net/SONiC#1071

Merged

Ashish1805 marked this pull request as draft May 31, 2024 04:19

DendroLabs mentioned this pull request Mar 17, 2026

[Link Event Damping] Add per port link event damper class. #1334

Open

DendroLabs mentioned this pull request Mar 17, 2026

[Link Event Damping] Add SelectablesTracker for timer management #1798

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[Link Event Damping] Add tracker to track the selectable timers used by link event damper#1323
Ashish1805 wants to merge 1 commit intosonic-net:masterfrom
Ashish1805:ledamping_seh

Ashish1805 commented Nov 17, 2023

Uh oh!

Ashish1805 commented Nov 18, 2023

Uh oh!

kcudnik commented Nov 18, 2023

Uh oh!

Uh oh!

kcudnik Nov 18, 2023

Uh oh!

Ashish1805 Nov 28, 2023

Uh oh!

kcudnik Nov 28, 2023

Uh oh!

kcudnik left a comment

Uh oh!

kcudnik commented Nov 18, 2023

Uh oh!

Ashish1805 commented Nov 28, 2023

Uh oh!

Junchao-Mellanox Nov 28, 2023

Uh oh!

kcudnik Nov 28, 2023

Uh oh!

DendroLabs commented Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Ashish1805 commented Nov 17, 2023

Uh oh!

Ashish1805 commented Nov 18, 2023

Uh oh!

kcudnik commented Nov 18, 2023

Uh oh!

Uh oh!

kcudnik Nov 18, 2023

Choose a reason for hiding this comment

Uh oh!

Ashish1805 Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

kcudnik Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

kcudnik left a comment

Choose a reason for hiding this comment

Uh oh!

kcudnik commented Nov 18, 2023

Uh oh!

Ashish1805 commented Nov 28, 2023

Uh oh!

Junchao-Mellanox Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

kcudnik Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

DendroLabs commented Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants