Switch attributes for tunnel ECN mode #1153
Conversation
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
inc/saiswitch.h
Outdated
| * @objects SAI_OBJECT_TYPE_TUNNEL_MAP | ||
| * @default empty | ||
| */ | ||
| SAI_SWITCH_ATTR_TUNNEL_ENCAP_ECN_MAPPERS, |
There was a problem hiding this comment.
The new API assumes one global configuration for all tunnel types. Meaning, if we have IP in IP tunnel and VXLAN tunnel, and we set this attribute, they will both use the same map.
Some ASICs have capability to set it per tunnel type. Meaning, having SAI_SWITCH_ATTR_TUNNEL_IPINIP_ENCAP_ECN_MAPPERS, SAI_SWITCH_ATTR_TUNNEL_VXLAN_ENCAP_ECN_MAPPERS,
SAI_SWITCH_ATTR_TUNNEL_MPLS_ECN_MAPPERS (similar to hash API where we have global configuration per traffic type SAI_SWITCH_ATTR_ECMP_HASH_IPV4, SAI_SWITCH_ATTR_ECMP_HASH_IPV4_IN_IPV4, etc.)
There was a problem hiding this comment.
Thats correct. Idea is to use global switch level configuration if there is a uniform ecn mode for given tunnel types for encap or decap.
This can always be overridden by the tunnel specific ECN mapper.
There was a problem hiding this comment.
Still we don't allow global per tunnel type
There can be several ways to configure :
- Global per all tunnels
- Global per tunnel type (meaning, all IP in IP tunnels, all VXLAN tunnels, etc.)
- Per specific tunnel (what we have today)
This PR is adding SAI ACL Google UT #1, but not adding Made SAI_BUFFER_POOL_ATTR_TH_MODE as create only #2 which is more flexible. I suggest adding Made SAI_BUFFER_POOL_ATTR_TH_MODE as create only #2 instead of SAI ACL Google UT #1
There was a problem hiding this comment.
Lets discuss this. Currently this PR is supporting (1)
There was a problem hiding this comment.
I think having per tunnel type level of default setting as suggested by @itaibaz is good which gives more granularity option to the application user. Perhaps we can still have an attribute that covers for ALL tunnel types as well such as SAI_SWITCH_ATTR_ALL_TUNNEL_ECN_MAPPERS that sets the default at switch level when there are no specific per Tunnel type nor at the per tunnel level.
There was a problem hiding this comment.
Thanks @gechiang @itaibaz for comments.
Looks like ECN mappers are easier to extend as it is an object list.
But ECN mode is of type sai_tunnel_decap_ecn_mode_t. So either we create these enums per tunnel type (sai_tunnel_decap/encp/_vxlan/ipinip/mpls_ecn_mode_t
or just leave the ecn mode to support model 1. What do you think?
There was a problem hiding this comment.
I would prefer per tunnel type attributes
Also, not sure I follow the usage of single ecn mappers attribute with object list. We can attach multiple maps to a single attrib, but how will we specify which map is valid for IP in IP and which one is valid for VXLAN ?
There was a problem hiding this comment.
@itaibaz can you confirm if this is what you are requesting.
- sai_tunnel_decap_vxlan_ecn_mode_t
- sai_tunnel_encap_vxlan_ecn_mode_t
- sai_tunnel_decap_ipinip_ecn_mode_t
- sai_tunnel_encap_ipinip_ecn_mode_t
- sai_tunnel_decap_mpls_ecn_mode_t
- sai_tunnel_encap_mpls_ecn_mode_t
There was a problem hiding this comment.
yes,
sai_tunnel_decap_vxlan_ecn_mode_t
sai_tunnel_encap_vxlan_ecn_mode_t
sai_tunnel_decap_ipinip_ecn_mode_t
sai_tunnel_encap_ipinip_ecn_mode_t
sai_tunnel_decap_mpls_ecn_mode_t
sai_tunnel_encap_mpls_ecn_mode_t
looks good
And also re ECN mappers, either we make it per tunnel type attribute
Or if using object list, please explain how to specify which tunnel type gets which map
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
| SAI_SWITCH_ATTR_TUNNEL_MPLS_ENCAP_ECN_MODE, | ||
|
|
||
| /** | ||
| * @brief Tunnel decap ECN Mode load method |
There was a problem hiding this comment.
Add IP in IP in beginning of comment
| SAI_SWITCH_ATTR_TUNNEL_MPLS_DECAP_ECN_MAPPERS, | ||
|
|
||
| /** | ||
| * @brief Tunnel encap ECN Mode load method |
There was a problem hiding this comment.
Add IP in IP in beginning of comment
| SAI_SWITCH_ATTR_TUNNEL_MPLS_ENCAP_ECN_MAPPERS, | ||
|
|
||
| /** | ||
| * @brief Tunnel mappers for decap |
There was a problem hiding this comment.
Add IP in IP in beginning of comment
| @@ -2256,6 +2432,158 @@ typedef enum _sai_switch_attr_t | |||
| */ | |||
| SAI_SWITCH_ATTR_SUPPORTED_FAILOVER_MODE, | |||
|
|
|||
| /** | |||
| * @brief Tunnel mappers for encap | |||
There was a problem hiding this comment.
Add IP in IP in beginning of comment
| /** | ||
| * @brief Defines tunnel encap ECN mode | ||
| */ | ||
| typedef enum _sai_tunnel_ipinip_encap_ecn_mode_t |
There was a problem hiding this comment.
Do we need to have enum per type, or can use a single enum ?
I think single enum would be good enough, but remember there is a prefix check, which maybe forces enum per type. Perhaps ask Kamil if a single enum can be used ?
|
please fix conflicts |
|
Please refer to #1173 for updated design |
This PR is the closure of discussion in #1153 There is switch level tunnel object which can be set for various attributes per tunnel type. Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
Add switch attributes for switch level configuration of tunnel ECN modes.
Encap and Decap ecn modes can be configured individually with separately assigned ECN mappers for user defined ECN mode.