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

FEC auto determination #1490

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

longhuan-cisco
Copy link
Contributor

FEC mode is a critical configuration for a port, which needs to be configured properly for the port to come up.

There are below scenarios that can end up with wrong FEC mode:

  1. In DPB(Dynamic Port Breakout) case, today's DPB CLI doesn't generate FEC config for newly created ports, FEC mode is default to none at SAI/SDK layer.
  2. In non-DPB case,
    • Some platforms have no FEC configured in CONFIG_DB by default. The FEC mode can be either default to none at SAI/SDK layer or manually configured by user who might not have enough domain knowledge.
    • Some platforms have default FEC defined in port_config.ini, which however might not be suitable for the specific port/optics on the system.

The feature in this document is to address the issue in both of above scenarios in a common platform-independent way, since the rule to determine FEC for a given port/optics is common for all platforms.

@longhuan-cisco
Copy link
Contributor Author

Add @shyam77git @jaganbal-a

@zhangyanzhao
Copy link
Collaborator

@lguohan
Copy link
Contributor

lguohan commented Oct 5, 2023

we need backward compatibility.

Add `determine-fec` module which can determine FEC mode based on common rule for a given port with a given optics. This module provides a `determine_fec` API which can be invoked in below use cases:
1. DPB use case: Enhance today's DPB CLI to automatically determine and configure FEC in CONFIG_DB for dynamically created ports, based on determine-fec module.
2. non-DPB use case: Add a user-triggered CLI `fec-auto-correct` to automatically determine and configure FEC in CONFIG_DB for existing ports, based on determine-fec module.
- Future plan: determine-fec module can be further enhanced to be integrated with xcvrd, which can be triggered automatically during transceiver insertion, without human intervention. (details TBD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the HLD as discussed in the community to capture the flow where FEC setting is generated by xcvrd based on media type and pushed to application DB.

> If a port has matched FEC entry in both above tables, then prefers FEC entry in first table. (Only exception: For `lane_speed=10, num_lane=1 or 4`, we prefer FEC entry in second table)


### Diagram For Different Use Cases
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the diagrams capturing the flow from xcvrd.
For the DPB use case, since xcvrd today listens to port breakout and generates settings, it would be better for xcvrd to use the same flow as during initialization to push the FEC setting for newly added ports in case of DPB

```

> [!NOTE]
> In the above use cases, automatically determined FEC mode is saved in running config, user needs to do ```config save``` to save it permanently.
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in the community review, the FEC settings should be added to app_db and not config_db. In addition there should be a global configuration knob to allow xcvrd auto setting FEC. This knob should be disabled by default to honor backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dgsudharsan
That's correct. We would be adding these two updates to the HLD

Also, we would update this HLD with following as well:

  • Remove CLI mention from the HLD and directly mention regular use-case i.e. xcvrd is the consumer of new FEC determination API (mentioned in this HLD)
  • Anytime, there is FEC config in config_db, it would supersede any existing or upcoming FEC setting.
    For this HLD it implies: Apply FEC determination API only if there is no FEC entry/setting in config_db.

Above tables can be defined as JSON file, and be loaded by determine-fec module. Platform can also override the default FEC mapping by providing its own JSON file.

> [!NOTE]
> If a port has matched FEC entry in both above tables, then prefers FEC entry in first table. (Only exception: For `lane_speed=10, num_lane=1 or 4`, we prefer FEC entry in second table)

Choose a reason for hiding this comment

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

First table defines the Optical media interface only. there can be on a single optical interface different types of electrical interface which define different FEC types.
for example: 100GBASE-DR (Clause 140) defines optical interface of 1 fiber lane.
Electrical interfaces that can be are (according to IEEE clause 140)
CAUI-4 (100G on 4 electrical lanes) - NO-FEC
100GAUI-2 (100G on 2 electrical lanes) - RS-FEC
100GAUI-1 (100G on 1 electrical lane) - RS FEC

This does not align with the statement in line 107 that " If a port has matched FEC entry in both above tables, then prefers FEC entry in first table".
FEC should be decided based on the Electrical interface (Second table) and not the first table in optical cable cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for example: 100GBASE-DR (Clause 140) defines optical interface of 1 fiber lane. Electrical interfaces that can be are (according to IEEE clause 140) CAUI-4 (100G on 4 electrical lanes) - NO-FEC 100GAUI-2 (100G on 2 electrical lanes) - RS-FEC 100GAUI-1 (100G on 1 electrical lane) - RS FEC

Just to confirm, for host side fec, were you saying we need to have the following mappings and it's standard?

optics_type=100G-DR, lane_speed=25, num_lanes=4, fec=none
optics_type=100G-DR, lane_speed=50, num_lanes=2, fec=rs
optics_type=100G-DR, lane_speed=100, num_lanes=1, fec=rs

FEC should be decided based on the Electrical interface (Second table) and not the first table in optical cable cases

I think we still need to rely on both optics_type(1st table) and electrical interface (2nd table).
For example, for the case of (lane_speed=25, num_lanes=4), fec can be either RS (for 100G CWDM4/100G PSM4/100G-CR/etc) or
none (for 100G-DR/100G-FR/etc)

Choose a reason for hiding this comment

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

I agree, there are some cases where on same lane_speed you can have different FEC results.
so it should based on inputs from both tables, but not just one of them

@zhangyanzhao
Copy link
Collaborator

@longhuan-cisco can you please help to add the code PRs by referring to #806 ? More info can be found from https://github.com/sonic-net/SONiC/wiki/Becoming-a-contributor

@zhangyanzhao
Copy link
Collaborator

@longhuan-cisco does this feature require SAI change?

@longhuan-cisco
Copy link
Contributor Author

@longhuan-cisco can you please help to add the code PRs by referring to #806 ? More info can be found from https://github.com/sonic-net/SONiC/wiki/Becoming-a-contributor

Some major updates need to be done in HLD as discussed in community review. And code PRs will be added after that.

@longhuan-cisco does this feature require SAI change?

According to the current plan, it's not required.

@zhangyanzhao
Copy link
Collaborator

code PR is not ready, move to backlog for future release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: MovedToBacklog
Development

Successfully merging this pull request may close these issues.

7 participants