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 link event damping sairedis port attributes. #1314

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

Ashish1805
Copy link
Contributor

  • Add link event damping sairedis port attributes.
  • These attributes will be used by OA to set the link event damping algorithm and AIED link event damping config.
  • Syncd uses these sairedis port attribute to receive the link event damping algorithm and AIED config to execute syncd-based link event damping.

HLD: sonic-net/SONiC#1071

@Ashish1805
Copy link
Contributor Author

Adding @Junchao-Mellanox for review.

@kcudnik
Copy link
Collaborator

kcudnik commented Oct 31, 2023

please fix aspell:

WARNING: Word 'AIED' is misspelled ../lib/sairedis.h
WARNING: Word 'validonly' is misspelled ../lib/sairedis.h
FAIL: aspellcheck.pl

lib/sairedis.h Outdated Show resolved Hide resolved
@Ashish1805 Ashish1805 force-pushed the ledamping_sairedis branch 2 times, most recently from df56d94 to fe445f5 Compare November 1, 2023 20:21
@Ashish1805
Copy link
Contributor Author

please fix aspell:

WARNING: Word 'AIED' is misspelled ../lib/sairedis.h
WARNING: Word 'validonly' is misspelled ../lib/sairedis.h
FAIL: aspellcheck.pl

Please let me know if you have any other comments/concerns. If everything looks good, can we merge this PR? Thank you.

@Ashish1805
Copy link
Contributor Author

HI @kcudnik, I dont have permission to merge PRs. Can you please merge this PR? Thank you.

@kcudnik
Copy link
Collaborator

kcudnik commented Nov 8, 2023

just one more question here, is this will be used internally by sairedis ? since i see you added custom range port attributes, or this will interact with SAI via sairedis ?

@Ashish1805
Copy link
Contributor Author

just one more question here, is this will be used internally by sairedis ? since i see you added custom range port attributes, or this will interact with SAI via sairedis ?

This will be used internally by sairedis, it will not interact with SAI. Basically swss will use these port attributes to set the link event damping config and link event damping algo (using sai_port_api->set_port_attribute() API) and these set requests will be consumed by sairedis internally to set the link event damping algo and config in the link event damping logic in syncd.

@kcudnik
Copy link
Collaborator

kcudnik commented Nov 8, 2023

Why then this state needs to be held inside sairedis and not OA directly ?

@Ashish1805
Copy link
Contributor Author

Why then this state needs to be held inside sairedis and not OA directly ?

There was couple of reasons:

  • When link events are sent from syncd, there can be multiple listeners to these events notifications coming from syncd (today it is OA but in future other components can listen to these events too) and if OA is doing damping, other listeners will receive the undamped events from syncd.
  • Keeping the damping logic in software stack as low as possible will reduce the number of notifications upper layer gets.
  • Adding the damping in syncd holds the events before any applications can get them.

We discussed this during HLD review and syncd implementation had advantages over OA implementation.

cc: @bhagatgit @mikeberesford

@kcudnik
Copy link
Collaborator

kcudnik commented Nov 12, 2023

ok sounds good

@Ashish1805
Copy link
Contributor Author

ok sounds good

Can you merge the PR if everything looks good? Thank you.

- Add link event damping sairedis port attributes.
- These attributes will be used by OA to set the link event damping
  algorithm and AIED link event damping config.
- Syncd uses these sairedis port attribute to receive the link event
  damping algorithm and AIED config to execute syncd-based link event
  damping.

HLD: sonic-net/SONiC#1071
@kcudnik kcudnik merged commit c532600 into sonic-net:master Nov 14, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants