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

[bfdorch] bfdorch needs to query SAI_BFD_SESSION_ATTR_PORT before programming it when port is not default #3051

Open
dgsudharsan opened this issue Feb 16, 2024 · 7 comments
Assignees

Comments

@dgsudharsan
Copy link
Collaborator

BFD configuration can specify ifname when creating the session or it can be set to default. When non default ifname is set, the attribute SAI_BFD_SESSION_ATTR_PORT will be programmed. However SAI library may not support this attribute.
Hence bfdorch should query support for this attribute and enable programming of non default interface only when this is supported

@dgsudharsan
Copy link
Collaborator Author

@prsunny FYI

@prsunny
Copy link
Collaborator

prsunny commented Feb 17, 2024

@kperumalbfn

@kperumalbfn
Copy link
Contributor

@dgsudharsan Currently bfdorch sets SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID as False. In that case, SAI_BFD_SESSION_ATTR_PORT is mandatory. How does ASIC determines determine the destination port if the SAI_BFD_SESSION_ATTR_PORT is not passed to SAI? Please clarify.

@dgsudharsan
Copy link
Collaborator Author

@kperumalbfn This flow is true when interface is non default https://github.com/sonic-net/sonic-swss/blob/97c7f3edbfedddceaa3bbac04f9d56e9bcad558c/orchagent/bfdorch.cpp#L437C9-L437C27

If a vendor SAI doesn't support SAI_BFD_SESSION_ATTR_PORT attribute, orchagent should return error without further calling SAI with create bfd session.
Currently there is no such check and the vendor SAI call will be invoked which will result in orchagent crash.

@kperumalbfn
Copy link
Contributor

@dgsudharsan as per the BFD SAI spec, BFD session port/src_mac/dst_mac fields are valid only when hw_lookup_valid is false. Current bfdorch.cpp sets 'hw_lookup_valid = false' for all BFD sessions.

https://github.com/opencomputeproject/SAI/blob/bc1d6ec90fb462c5171dbaa1aedcab1aac890ce9/inc/saibfd.h#L179

If vendor SAI uses h/w lookup to derive the egress port and BFD packet's L2 rewrite, then we need to update the bfdorch.cpp as well based on the capability check. Could you confirm the behavior?

@dgsudharsan
Copy link
Collaborator Author

@kperumalbfn The logic in bfdorch is to set SAI_BFD_SESSION_ATTR_PORT when alias is not "default". It doesn't check if the attribute is supported.

if (alias != "default")

Since this can be SAI vendor implementation specific and not mandatory attribute as well, if SAI has not implemented orchagent will crash.
So if the user sets alias as not default, before setting SAI_BFD_SESSION_ATTR_PORT it is better to query if the attribute is supported and if not, log error message in orchagent and avoid programming SAI

1 similar comment
@dgsudharsan
Copy link
Collaborator Author

@kperumalbfn The logic in bfdorch is to set SAI_BFD_SESSION_ATTR_PORT when alias is not "default". It doesn't check if the attribute is supported.

if (alias != "default")

Since this can be SAI vendor implementation specific and not mandatory attribute as well, if SAI has not implemented orchagent will crash.
So if the user sets alias as not default, before setting SAI_BFD_SESSION_ATTR_PORT it is better to query if the attribute is supported and if not, log error message in orchagent and avoid programming SAI

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

No branches or pull requests

3 participants