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

Class Based Forwarding HLD #796

Merged
merged 4 commits into from
Mar 23, 2022
Merged

Class Based Forwarding HLD #796

merged 4 commits into from
Mar 23, 2022

Conversation

abanu-ms
Copy link
Contributor

@abanu-ms abanu-ms commented Jun 8, 2021

This is the spec for the Class Based Forwarding feature.

Code PRs:
swss-common: sonic-net/sonic-swss-common#525
sairedis: sonic-net/sonic-sairedis#909
swss: sonic-net/sonic-swss#1963
buildimage: sonic-net/sonic-buildimage#8689
utilites: sonic-net/sonic-utilities#1799

@abanu-ms abanu-ms changed the title [cbf] Added CBF spec Class Based Forwarding spec Jun 8, 2021
@abanu-ms abanu-ms changed the title Class Based Forwarding spec Class Based Forwarding HLD Jun 8, 2021
@anshuv-mfst
Copy link
Collaborator

hi @ashutosh-agrawal - please review the feature, thanks.

;Status: Mandatory
key = CLASS_BASED_NEXT_HOP_GROUP_TABLE:string ; arbitrary string identifying the class based next hop group, as determined by the programming application.
members = NEXT_HOP_GROUP_TABLE.key, ; one or more indexes within NEXT_HOP_GROUP_TABLE, separated by “,”
class_map = number:number, ; one or more mapping from Forwarding Class to index in "members" field to use as NHG, separated by ","
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Aug 3, 2021

Choose a reason for hiding this comment

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

Why can't we have DSCP/EXP to NH mapping configs and use the same in a ROUTE_TABLE from App and then use a config-DB-based mapping for DSCP/EXP to FC before programming the HW? This way, App need not be aware of internal FC mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would limit flexibility in a couple of ways, so I think the current proposal is better in general.

  • There is SAI support for setting the FC via ACL rules as well. We're not adding support for that to SONiC at this time, but shouldn't prevent it by making this map go all the way from DSCP/EXP to NH in one go
  • The DSCP/EXP to FC maps can potentially be configured per-port, which also wouldn't work with this proposed map.

doc/cbf/cbf_hld.md Outdated Show resolved Hide resolved
;Status: Mandatory
key = DSCP_TO_FC_MAP_TABLE:string ; arbitrary string identifying the name of the map.
dscp_value = 1*DIGIT
fc_value = 1*DIGIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add SONiC YANG changes for config-DB tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please point me to the YANG for DSCP_TO_TC_MAP_TABLE to have an example? I'm not sure where to look for it.

@TACappleman
Copy link
Contributor

Hi @venkatmahalingam thanks for the feedback so far. We've got a few queries on some of these - could you have a look at them this week please?

```
This feature enables opeartors, among other things, to send the important (foreground) traffic through the shortest path, while sending the background traffic through longer paths to still give it some bandwidth instead of using QoS queues which may block background traffic from getting bandwitdh.

These new class based next hop groups are allowed thanks to the changes in https://github.com/opencomputeproject/SAI/pull/1193, which allow a next hop group object to also have other next hop group objects as members of the group along with the next hop objects. The way such a next hop group works is that a packet which has a Forwarding Class value of X will be matched against an appropriate member of this group, selected based on the Forwarding Class value thanks to the "class_map" property of the group. As an example, given the CBF group with members Nhg1, Nhg2 and Nhg3 and a class map of FC 0 -> Nhg1, FC 1 -> Nhg2 and FC 3 -> Nhg3, a packet which has an FC value of 0 will be forwarded using Nhg1. Note that multiple FC values can point to the same member, but a single FC value can't be mapped to more than one member.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any plan to update FC via ACL using SAI attribute SAI_ACL_ENTRY_ATTR_ACTION_SET_FORWARDING_CLASS? If so, please update the HLD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not going to offer this kind of support for now.

nexthop = *prefix, ;IP addresses separated “,” (empty indicates no gateway)
ifname = *PORT_TABLE.key, ; zero or more separated by “,” (zero indicates no interface)
blackhole = BIT ; Set to 1 if this route is a blackhole (or null0)
nexthop_group = NEXT_HOP_GROUP_TABLE.key or CLASS_BASED_NEXT_HOP_GROUP_TABLE.key ; index within the NEXT_HOP_GROUP_TABLE or CLASS_BASED_NEXT_HOP_GROUP_TABLE, optionally used instead of nexthop and intf fields
Copy link
Contributor

Choose a reason for hiding this comment

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

@TACappleman Could you also add the changes to Sonic orchagent(if any) when some or all members of CBF NHG in ROUTE_TABLE are not resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a ROUTE_TABLE entry references a CBF NHG which hasn't been added yet by the user application, the behavior is the same as for a normal NHG - the route entry will remain in the Consumer's queue for the event of the NHG being created at some point which will allow the route entry to be created.

If a ROUTE_TABLE entry references a CBF NHG which has been added by the user application, but doesn't exist in the ASIC_DB (because the NH objects weren't created yet in the ASIC_DB perhaps), the same thing as in the first case will happen. RouteOrch will keep the entry in the queue, trying to create it. Beyond that, the CBF NHG handler will also keep trying to create the CBF NHG. This behavior is covered by the following statement from the HLD:
otherwise the task will be kept in the process queue for the event of the missing member(s) being created which would allow the class based next hop group to be created.

- If a packet is not matched against an FC value and the route for its destination references a CBF NHG, the packet will be dropped
- If a packet is matched against an FC value and the route for its destination does not reference a CBF NHG, the packet will use the route's NH
- If a packet is matched against an FC value and the route for its destination references a CBF NHG which maps the packet's FC value, the packet will use the mapped NHG
- If a packet is matched against an FC value and the route for its destination references a CBF NHG which doesn't map the packet's FC value, the packet will be dropped
Copy link
Contributor

Choose a reason for hiding this comment

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

Any CBF specific drop reason for the above drop 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.

Both drop cases have the reasoning that a CBF NHG needs to forward a packet based on its FC value. If that FC value doesn't exist or the FC value isn't mapped by the CBF NHG to a specific NH, we can't make any assumptions on which NH we should choose. The user user application should make sure to provide a correct mapping for each CBF NHG and to have the correct CBF maps configured.

doc/cbf/cbf_hld.md Show resolved Hide resolved
@TACappleman
Copy link
Contributor

@zhangyanzhao can this be merged?

@zhangyanzhao
Copy link
Collaborator

@TACappleman sonic-net/sonic-swss#1963 #1963 is marked as a code PR but not merged yet. What is the plan for that PR?

@TACappleman
Copy link
Contributor

We have a review from Intel which we've almost finished dealing with, and are still awaiting an internal review. Expect to get it merged this week

rlhui pushed a commit to sonic-net/sonic-swss that referenced this pull request Nov 16, 2021
* [cbf] Added initial CBF support (#2)

* Added CBF NHG support to NhgOrch
* Added NhgMapOrch to handle DSCP_TO_FC and EXP_TO_FC tables
* Added UT for the new NhgOrch function and NhgMapOrch

Support sonic-net/SONiC#796

Co-authored-by: Alexandru Banu <v-albanu@microsoft.com>
TACappleman added a commit to Metaswitch/sonic-swss that referenced this pull request Dec 13, 2021
* [cbf] Added initial CBF support (#2)

* Added CBF NHG support to NhgOrch
* Added NhgMapOrch to handle DSCP_TO_FC and EXP_TO_FC tables
* Added UT for the new NhgOrch function and NhgMapOrch

Support sonic-net/SONiC#796

Co-authored-by: Alexandru Banu <v-albanu@microsoft.com>
@zhangyanzhao
Copy link
Collaborator

All code PRs are merged, HLD is reviewed in community. Merge it.

@zhangyanzhao zhangyanzhao merged commit 2e585d8 into sonic-net:master Mar 23, 2022
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