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.

Sign up for GitHub

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

Jump to bottom

[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

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

wants to merge 1 commit into from

Conversation

Ashish1805
Copy link
Contributor

  • 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
Copy link
Contributor Author

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
Copy link
Collaborator

kcudnik commented Nov 18, 2023

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 !

kcudnik
kcudnik reviewed Nov 18, 2023
View reviewed changes
syncd/SelectablesTracker.h
Comment on lines +29 to +49
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 *>;
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

kcudnik
kcudnik requested changes Nov 18, 2023
View reviewed changes
Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

address comments

@kcudnik
Copy link
Collaborator

kcudnik commented Nov 18, 2023

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 Damping] Add tracker to track the selectable timers used by
0fe378f 
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
Copy link
Contributor Author

Adding @Junchao-Mellanox for review.

Junchao-Mellanox
Junchao-Mellanox reviewed Nov 28, 2023
View reviewed changes
syncd/SelectablesTracker.cpp
}

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()

@Ashish1805 Ashish1805 mentioned this pull request Dec 7, 2023
Merged
@Ashish1805 Ashish1805 marked this pull request as draft May 31, 2024 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Junchao-Mellanox Junchao-Mellanox Junchao-Mellanox left review comments

@kcudnik kcudnik kcudnik requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ashish1805 @kcudnik @Junchao-Mellanox