Skip to content

HLD Port FEC BER #1829

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

Merged
merged 4 commits into from
Nov 19, 2024
Merged

HLD Port FEC BER #1829

merged 4 commits into from
Nov 19, 2024

Conversation

vincentpcng
Copy link
Contributor

@vincentpcng vincentpcng commented Oct 14, 2024

What I did

Add design proposal for Port FEC BER feature

Summary of Changes:

Enhance the collect and compute the FEC BER on each port.
Modify the port_rates.lua , portstat.py and netstat.py

PR Title State Context
[sonic-utilities] Add port FEC BER show changes PR Status PR Checks
[sonic-swss] Add port FEC BER feature swss PR Status PR Checks
[sonic-mgmt] Add the port FEC BER mgmt test PR Status PR Checks

@vincentpcng vincentpcng marked this pull request as ready for review October 15, 2024 22:42
@qinchuanares
Copy link
Contributor

When the port is down or flapping, or even postFEC error > 0, preFEC BER calculation does not make sense.

After link flap or link down, when link is stably up, the preFEC BER calculation uses counters reset from 0 at link up.

@vincentpcng

This comment was marked as duplicate.

@vincentpcng vincentpcng reopened this Oct 30, 2024
@vincentpcng
Copy link
Contributor Author

vincentpcng commented Oct 30, 2024

hit the wrong button and accidental closed the PR, reopen it.

@vincentpcng
Copy link
Contributor Author

When the port is down or flapping, or even postFEC error > 0, preFEC BER calculation does not make sense.

After link flap or link down, when link is stably up, the preFEC BER calculation uses counters reset from 0 at link up.

Agree, the measurement could be compromised if the link is not stable, like flapping occur, etc

@Junchao-Mellanox
Copy link
Contributor

The feature only capture the realtime port FEC BER. Is it possible to capture any history port FEC BER issue so that we will be able to get some debug information when an issue only happens in a short period?

@vincentpcng
Copy link
Contributor Author

The following code review PR is for the implementation of this HLD
(1) sonic-net/sonic-swss#3363
(2) sonic-net/sonic-utilities#3607
(3) sonic-net/sonic-mgmt#15481

@vincentpcng
Copy link
Contributor Author

vincentpcng commented Nov 12, 2024

Review feedback , the following key points were mentions

(1) can we add min , max, average BER
(2) what about future serdes , can we use user data rate when compute the BER.
(3) what about other devices support
(4) the statistical average 1E-8

response:
(1) we can add the min, max, and average to the show command... this can be done as a future enhancement
(2) unless IEEE defined the enoding, otherwise we dont know (i) what the future serdes speed, (ii) what type of encoding were use in it, hence can;t be done.
One suggestion is to use the user data rate for computing. This is a possible trade off. However, the resulted BER will become not a true reflection. The community future suggest can we add more columns and use the "user data rate".
This will be left as future enhancement discussion
(3) This feature is as stated, require the SAI support. If SAI layer can provide the counter, than we should be able to support the devices.
(4) when calculate the uncorrectable BER (post FEC), the frame were dropped, we have no idea how many error is in the frame. So we can take the best, worst or statistical average for the calculation. During the document review the 1e-8 statistically average were recommended for it.

Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

@vincentpcng can you please link all the code PRs to the HLD?

@vincentpcng
Copy link
Contributor Author

I added the code PR(s) links before the meeting minute comments...

Let me include it again here

The following code review PR is for the implementation of this HLD
(1) sonic-net/sonic-swss#3363
(2) sonic-net/sonic-utilities#3607
(3) sonic-net/sonic-mgmt#15481

@prgeor prgeor merged commit a702be5 into sonic-net:master Nov 19, 2024
1 check 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.

7 participants