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

Skip default IPv6 route with prefix zero and fix IPv6 prefix bug #565

Merged
merged 6 commits into from
Jun 20, 2024

Conversation

Pterosaur
Copy link
Collaborator

@Pterosaur Pterosaur commented May 19, 2024

1.To set a default IPv6 route with the prefix 0, we will get following error logs. It's caused by the limitation of p4lang: https://github.com/p4lang/PI/blob/24e0a3c08c964e36d235973556b90e0ae922b894/proto/frontend/src/device_mgr.cpp#L2242-L2246 .

2024 Jun  3 15:26:21.033130 vlab-01 ERR syncd#syncd_dash: e:- mutateTableEntry: GRPC ERROR[2]: , #010#002#032¡#001#012#037type.googleapis.com/p4.v1.Error#022~#010#003#022gInvalid reprsentation of 'don't care' LPM match, omit match field instead of using a prefix length of 0#032#021ALL-sswitch-p4org
2024 Jun  3 15:26:21.033130 vlab-01 ERR syncd#syncd_dash: e:- mutateTableEntry: GRPC call Write::INSERT ERROR: table_id: 49279256 match { field_id: 1 lpm { value: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" } } action { action { action_id: 32404057 params { param_id: 1 value: "\000\000" } params { param_id: 2 value: "\000\000" } } }

In this case, I just ignore the default IPv6 route to skip this limitation.

  1. Fix IPv6 prefix length function bug.
    There are two issues on the IPv6 prefix:
  • when we calculate the prefix length from the mask, we should consider the trailing zeros from high address space, aka, the end of mask array, of SAI IPv6 attribution. So, I revised this issue by reversing the IPv6 mask array.
  • Because the function leadingNonZeroBits returns the exact length of prefix, we shouldn't directly subtract it from 129. For example, if the mask is 0xffffffffffffffffffffffff, the original implementation will get 129-0-32=97, it is obviously wrong. The correct answer is 128. So, I fix this bug.

Signed-off-by: Ze Gan <ganze718@gmail.com>
@r12f r12f marked this pull request as ready for review May 20, 2024 17:46
@r12f
Copy link
Collaborator

r12f commented May 20, 2024

Hi @Pterosaur , do you mind to add some description here to describe the issue and how we are fixing it?

@@ -268,6 +269,10 @@ namespace dash
case SAI_IP_ADDR_FAMILY_IPV6:
{
uint8_t ip[16];
if (std::is_same<T, p4::v1::FieldMatch_LPM>::value) {
// BMv2 cannot support IPv6 LPM with prefix
return SAI_STATUS_NOT_SUPPORTED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this the BMv2's limitation or it is our implementation limit?

also i am a bit hesitated that we should check and fail here, because of 2 reasons:

  • It is a utility function and should not have any knowledge on the scenarios
  • Also the type check is a bit hacky.

IMO, if this is something we don't support, the change in the template should be good enough.

@Pterosaur Pterosaur marked this pull request as draft May 21, 2024 11:39
@Pterosaur
Copy link
Collaborator Author

Hi @Pterosaur , do you mind to add some description here to describe the issue and how we are fixing it?

Hi @r12f , Thanks for your suggestion. This PR is just for testing right now, So it looks hacky and drafted. I will refactor it after confirmed.

@Pterosaur Pterosaur force-pushed the ipv6_bypass branch 2 times, most recently from 5078c25 to 999f4f1 Compare June 3, 2024 13:25
Signed-off-by: Ze Gan <zegan@microsoft.com>
@Pterosaur Pterosaur changed the title Add IPv6 bypass to dash SAI Skip default IPv6 route with prefix 0 Jun 4, 2024
@Pterosaur Pterosaur changed the title Skip default IPv6 route with prefix 0 Skip default IPv6 route with prefix zero Jun 4, 2024
Signed-off-by: Ze Gan <zegan@microsoft.com>
@Pterosaur Pterosaur changed the title Skip default IPv6 route with prefix zero Skip default IPv6 route with prefix zero and prefix bug Jun 4, 2024
Signed-off-by: Ze Gan <zegan@microsoft.com>
@Pterosaur Pterosaur marked this pull request as ready for review June 4, 2024 12:45
@r12f
Copy link
Collaborator

r12f commented Jun 4, 2024

hi Ze, do you mind to add some explanation on what bug we are fixing in this change and how?

Signed-off-by: Ze Gan <zegan@microsoft.com>
@Pterosaur Pterosaur changed the title Skip default IPv6 route with prefix zero and prefix bug Skip default IPv6 route with prefix zero and fix IPv6 prefix bug Jun 5, 2024
@Pterosaur Pterosaur requested a review from r12f June 5, 2024 15:42
@Pterosaur
Copy link
Collaborator Author

hi Ze, do you mind to add some explanation on what bug we are fixing in this change and how?

@r12f Thanks for your suggestion, Please check it again.

@KrisNey-MSFT
Copy link
Collaborator

@r12f - looks like a request for you to re-check after edits :)

@r12f r12f merged commit 8813f26 into sonic-net:main Jun 20, 2024
4 checks passed
@KrisNey-MSFT
Copy link
Collaborator

KrisNey-MSFT commented Jun 27, 2024

This is to ensure bmv2 can launch, but is a temporary patch. Not valid in LPM, and it fails.
@r12f will designate someone to file an Issue in P4 Runtime GitHub (leading 0's) if we feel it is related here.
We need a volunteer to fix this if we can figure it out.

https://github.com/p4lang/p4runtime

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.

3 participants