Skip to content

srv6: flow label control #35498

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

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Conversation

akaliwod
Copy link

@akaliwod akaliwod commented Oct 23, 2024

This commit introduced IPv6 flow label control for SRv6 encapsulated packets. Currently IPv6 flow label is set to 0x0 value.

IPv6 flow label is used to perform ECMP load-balancing in the core of the network. Setting flow label provides entropy and allows packet flows to be correctly load balanced.

The commit introduces configurable flag that keeps the default behavior of flow label set to 0x0 value. As on option it enables TCP/UDP port based basic calculation of the flow label value that is set in the IPv6 header in case of SRv6 packet flow.

Fixes: #21961

Fix missing flowlabel hash on SRv6 traffic.

@akaliwod akaliwod requested review from a team as code owners October 23, 2024 06:34
@akaliwod akaliwod requested a review from ldelossa October 23, 2024 06:34
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 23, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 23, 2024
@YutaroHayakawa YutaroHayakawa self-requested a review October 24, 2024 07:58
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution! A couple of comments.

@akaliwod akaliwod force-pushed the akaliwod/srv6/flow-label branch 2 times, most recently from 975a975 to 332fe72 Compare October 28, 2024 15:01
@akaliwod akaliwod force-pushed the akaliwod/srv6/flow-label branch from 332fe72 to 35ea7cf Compare October 29, 2024 08:33
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Now the change looks good to me. Thanks!

@YutaroHayakawa YutaroHayakawa added the release-note/misc This PR makes changes that have no direct user impact. label Oct 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 29, 2024
@YutaroHayakawa
Copy link
Member

/test

@akaliwod akaliwod force-pushed the akaliwod/srv6/flow-label branch from 35ea7cf to fc6834a Compare October 29, 2024 11:35
@akaliwod
Copy link
Author

Going back to 16lsb implementation of flow label as per advice from Ahmed based on the ietf draft and intrustry best practices

@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Oct 29, 2024

@ahsalam As I said out of band, I don't have any objection against this change, but could you elaborate more about the context here (for example, the link to the draft or vendor's docuemnt)? We need to leave the clear context since it's OSS. If we say only using the 16bit is the best practice of the industry, we need to show the reasonable evidence.

Is this the one? https://www.ietf.org/archive/id/draft-filsfils-6man-structured-flow-label-02.html

@ahsalam
Copy link

ahsalam commented Oct 29, 2024

@ahsalam As I said out of band, I don't have any objection against this change, but could you elaborate more about the context here (for example, the link to the draft or vendor's docuemnt)? We need to leave the clear context since it's OSS. If we say only using the 16bit is the best practice of the industry, we need to show the reasonable evidence.

Is this the one? https://www.ietf.org/archive/id/draft-filsfils-6man-structured-flow-label-02.html

Hi @YutaroHayakawa @akaliwod Yes, this is the draft.

This document defines the IPv6 Structured Flow Label. The document demonstrated the seamless change to [RFC6437] and explained the benefits of the solution and illustrated the use-cases.

Instead of using FL as a single 20-bit entropy structure, this draft updates [[RFC6437] and defines the 20-bit FL field as a structure of two fields:

  • FLC: 4-bit per-packet control bits for generic application marking (e.g., group-based policy)
  • FLE: 16-bit per-flow entropy (equivalent to [[RFC6437] definition)

The draft explained the Seamless Migration from RFC6437. Also Cisco and Broadcom report that the norm for their products:

  • do not set entropy in the 4 most-specific bits of the FL field
  • do not use the 4 most-specific bits of the FL as input for ECMP hash

@YutaroHayakawa
Copy link
Member

Thanks for your explanation! I think we don't have any risk for following that convention by default. 16bits still provides an enough ECMP/RSS entropy and we can always introduce a flag to switch the behavior if needed.

@ldelossa ldelossa removed their request for review October 29, 2024 14:05
@dylandreimerink dylandreimerink removed request for a team and dylandreimerink October 29, 2024 15:49
@YutaroHayakawa
Copy link
Member

/test

@YutaroHayakawa
Copy link
Member

Looks like the ci-e2e-upgrade is failing pretty consistently. Let me check.

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I'd consider this a bugfix and would thus apply label release-note/bug and backport. @YutaroHayakawa What do you think?

@pchaigno
Copy link
Member

@akaliwod Rebasing against the latest main branch might help improve CI status. Your branch is currently one week old, which in Cilium's case is quite old.

@pchaigno pchaigno removed the request for review from a team October 30, 2024 09:47
@pchaigno pchaigno added the feature/srv6 Impacts the SRv6 feature. label Oct 30, 2024
@YutaroHayakawa YutaroHayakawa added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch and removed release-note/misc This PR makes changes that have no direct user impact. labels Oct 30, 2024
@YutaroHayakawa
Copy link
Member

I'd consider this a bugfix and would thus apply label release-note/bug and backport. @YutaroHayakawa What do you think?

Yep, agree. I put the label.

This commit introduced IPv6 flow label control for SRv6 encapsulated packets. Currently IPv6 flow label is set to 0x0 value.

IPv6 flow label is used to perform ECMP load-balancing in the core of the network. Setting flow label provides entropy and allows packet flows to be correctly load balanced.

The commit uses bfp_get_hash_recalc helper function to populate ctx->hash field and uses the result of helper to set flow label based on 16 lsb of the hash.

There is no configurable flag to control this behavior.

Additional change in bpf/lib/common.h to use get_hash(ctx) helper function instead of get_hash_recalc. Working assumption is that hash calculation is performed once.

Signed-off-by: Arkadiusz Kaliwoda (akaliwod) <akaliwod@cisco.com>
@akaliwod akaliwod force-pushed the akaliwod/srv6/flow-label branch from fc6834a to 4057d61 Compare October 30, 2024 14:26
@harsimran-pabla
Copy link
Contributor

/test

@pchaigno pchaigno enabled auto-merge October 30, 2024 16:03
@pchaigno pchaigno added this pull request to the merge queue Oct 30, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 30, 2024
Merged via the queue into cilium:main with commit f4988cb Oct 30, 2024
64 checks passed
@joamaki joamaki mentioned this pull request Nov 5, 2024
23 tasks
@joamaki joamaki added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Nov 5, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. feature/srv6 Impacts the SRv6 feature. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SRv6: Set the outer IPv6 flowlabel hash
7 participants