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.

Sign up for GitHub

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

Jump to bottom

User defined VxLAN UDP source port calculation #1205

Merged
merged 1 commit into from Apr 21, 2021
Merged

User defined VxLAN UDP source port calculation #1205

merged 1 commit into from Apr 21, 2021

Conversation

marian-pritsak
Copy link
Contributor

No description provided.

itaibaz
itaibaz reviewed Mar 17, 2021
View reviewed changes
inc/saiswitch.h Outdated Show resolved Hide resolved
inc/saiswitch.h Outdated Show resolved Hide resolved
itaibaz
itaibaz reviewed Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@itaibaz itaibaz left a comment

Choose a reason for hiding this comment

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

should we also add per tunnel attrib ?
as original attributes are both per {switch, tunnel type} and {tunnel}
otherwise, we may need to specify this mode is not per tunnel

@JaiOCP
Copy link
Contributor

JaiOCP commented Mar 17, 2021

Have you looked at PR#1173. It added following attributes
/**

  • @brief Defines VXLAN tunnel UDP source port mode
    /
    typedef enum _sai_tunnel_vxlan_udp_sport_mode_t
    {
    /    *

    • @brief User define value
      */
      SAI_TUNNEL_VXLAN_UDP_SPORT_MODE_USER_DEFINED,

    /**

    • @brief RFC6335 Computed hash value in range 49152-65535
      */
      SAI_TUNNEL_VXLAN_UDP_SPORT_MODE_EPHEMERAL,
      } sai_tunnel_vxlan_udp_sport_mode_t;

@marian-pritsak
Copy link
Contributor Author

should we also add per tunnel attrib ?
as original attributes are both per {switch, tunnel type} and {tunnel}
otherwise, we may need to specify this mode is not per tunnel

It doesn't make sense at the moment to have it per tunnel. If anyone will come up with the use case, it can be added.

@marian-pritsak
Copy link
Contributor Author

Have you looked at PR#1173. It added following attributes
/**

  • @brief Defines VXLAN tunnel UDP source port mode
    _/
    typedef enum sai_tunnel_vxlan_udp_sport_mode_t
    {
    /    *

    • @brief User define value
      */
      SAI_TUNNEL_VXLAN_UDP_SPORT_MODE_USER_DEFINED,

    /**

    • @brief RFC6335 Computed hash value in range 49152-65535
      */
      SAI_TUNNEL_VXLAN_UDP_SPORT_MODE_EPHEMERAL,
      } sai_tunnel_vxlan_udp_sport_mode_t;

"Ephemeral" is not flexible enough

@marian-pritsak
Copy link
Contributor Author

should we also add per tunnel attrib ?
as original attributes are both per {switch, tunnel type} and {tunnel}
otherwise, we may need to specify this mode is not per tunnel

It doesn't make sense at the moment to have it per tunnel. If anyone will come up with the use case, it can be added.

I misunderstood a comment - yes, it should be in both since the enum is shared

lguohan
lguohan reviewed Mar 25, 2021
View reviewed changes
inc/saiswitch.h Outdated
/**
* @brief User defined bit range of computed hash
*/
SAI_TUNNEL_VXLAN_UDP_SPORT_MODE_USER_DEFINED_HASH,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be merged into user_defined mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@marian-pritsak
User defined VxLAN UDP source port calculation
e7999ce 
Signed-off-by: Marian Pritsak <marianp@mellanox.com>
@rlhui
Copy link
Collaborator

rlhui commented Apr 1, 2021

@JaiOCP - would you like to review/approve?

@lguohan
Copy link
Collaborator

lguohan commented Apr 1, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JaiOCP
Copy link
Contributor

JaiOCP commented Apr 1, 2021

@JaiOCP - would you like to review/approve
Changes looks good

@rlhui rlhui merged commit fafbf60 into opencomputeproject:master Apr 21, 2021
@richardyu-ms richardyu-ms mentioned this pull request Apr 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itaibaz itaibaz itaibaz left review comments

@lguohan lguohan lguohan approved these changes

@kcudnik kcudnik kcudnik approved these changes

@JaiOCP JaiOCP JaiOCP approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@marian-pritsak @JaiOCP @rlhui @lguohan @itaibaz @kcudnik