LPM Masking: Fix masking for fields with bitwidth non divisable by 8.

[dorakerman]

When prefix and/or bitwidth are not aligend to size of a byte, we must make sure the mask is applied correctly to all bits in field. Until now, the logic expected to find maximum of one byte that needs special masking, and that masking is relative to the "beginning" (MSB bits) of the bytes. This caused issues with fields that are smaller than a byte. Another issue, some fields and prefixed might need some bits of a the "first" and "last" byte in the prefix to be masked, for example: Field bitwidth = 20
Prefix length = 16
Mask should be = 0b11111111111111110000
Since we are representing in byte arrays, in bytes we get -> 00001111 11111111 11110000 (24 bits, 3 bytes)

Old implmentation disregarded the first 2 bytes and only masked the last byte (0b11110000).

Both issues are fixed in this commit, by this flow:

1. Utilizing prefix byte array, which is aligned to number of bytes needed to represent bitwidth in bytes.
2. Creating a mask for entire field, according to prefix length.
3. Applying it to all bits in prefix length.

Issue: #146

[jafingerhut]

If I understand correctly:

(1) When the P4Runtime API has an exact or ternary field with width W bits that is not a multiple of 8, the match value (and for ternary fields, the mask) are represented by the value being in the least significant W bits of a sequence of B=ceiling(W/8) bytes, i.e. the most significant (8*B-W) bits of the most significant byte must be 0.

(2) The same is true for a value of an lpm match field, too, regardless of the prefix length.

(3) Without the change in this PR, the p4runtime-shell seems to be expecting the value of such an lpm field to be aligned differently than that, and you want to change p4runtime-shell it so it more closely matches what the P4Runtime API messages require.

Please correct me if I'm have any of that wrong.

[jafingerhut]

(2) The same is true for a value of an lpm match field, too, regardless of the prefix length.

Out of curiosity, have you tested with the BMv2 software switch with a table that has an lpm field to confirm whether it expects this alignment of values for lpm fields when configuring it via P4Runtime API messages?

If not, would you be willing to try that out? I ask mainly because it would be nice to know that we are keeping all of these pieces consistent with each other for this scenario.

[dorakerman]

Hi @jafingerhut , sorry for delay.

Yes I believe you are correct. Also, if I understood your question correctly, this issue was found while testing, and passed with new implementation :).

[jafingerhut]

Hi @jafingerhut , sorry for delay.

Yes I believe you are correct. Also, if I understood your question correctly, this issue was found while testing, and passed with new implementation :).

With what target device did you find the issue while testing, and passed with the new implementation?

I ask because I just tried an unmodified latest BMv2 software switch with latest P4Runtime API source code, with a 20-bit key field, and it seems to completely lose some key bits when inserting entries into such a table, perhaps because of assumptions within the BMv2 implementation that the 20-bit key must be left-shifted within the least significant 24 bits of the key.

If I am interpreting my test results correctly on BMv2, that sounds like it is probably a bug specific to the BMv2 implementation of longest-prefix match tables that have a bit width that is not a multiple of 8 bits.

[jafingerhut]

Hi @jafingerhut , sorry for delay.
Yes I believe you are correct. Also, if I understood your question correctly, this issue was found while testing, and passed with new implementation :).

With what target device did you find the issue while testing, and passed with the new implementation?

I ask because I just tried an unmodified latest BMv2 software switch with latest P4Runtime API source code, with a 20-bit key field, and it seems to completely lose some key bits when inserting entries into such a table, perhaps because of assumptions within the BMv2 implementation that the 20-bit key must be left-shifted within the least significant 24 bits of the key.

If I am interpreting my test results correctly on BMv2, that sounds like it is probably a bug specific to the BMv2 implementation of longest-prefix match tables that have a bit width that is not a multiple of 8 bits.

Or, my failing results are because of exactly the issue that you are fixing here, because my testing was using the unmodified p4runtime-shell for installing entries :-)

I will test locally with your poposed changes soon-ish, and let you know my results.

[jafingerhut]

@dorakerman I tried using your proposed changes here for p4runtime-shell with my test, and latest BMv2 and PI repositories as that target device (the PI repository provides some or all of the P4Runtime API server implementation in BMv2, I believe).

There appears to be a check in the PI repository that assumes longest-prefix match keys are left-adjusted within a whole number of bytes, here: https://github.com/p4lang/PI/blob/main/proto/frontend/src/common.cpp#L152

If you were able to get a test to pass with your proposed changes, then I am guessing that you are not using the code in this repository to implement a P4Runtime server: https://github.com/p4lang/PI

Is that true that you do not use the code in that repository?

Out of curiosity, does that mean that you are testing with a P4Runtime server implementation developed independently of that PI repository code?

Do you happen to know whether it is proprietary or open source? I do not need to know -- mainly curiosity on my part.

@antoninbas Does it sound reasonable to you that if there are multiple places in p4lang repositories that currently assume that LPM keys are "left-adjusted" within a whole number of bytes

(i.e. a 20-bit LPM key would be in bit positions 11111111 11111111 1111000 binary of 3 consecutive bytes)

that they should all be changed so that the keys are "right-adjusted" within a whole number of bytes

(i.e. a 20-bit LPM key would be in bit positions 00001111 11111111 11111111 binary of 3 consecutive bytes)

I believe the latter positioning of the key within 3 bytes would be consistent with how P4Runtime encodes exact and ternary match fields since the beginning of P4Runtime API's definition and implementation. I suspect most people have only used LPM key fields that are multiples of 8 bits before now (e.g. 32-bit IPv4 address, 128-bit IPv6 address, or 64-bit half of an IPv6 address), so this issue was not noticed for a long time.

[antoninbas]

@jafingerhut LPM values should be valid bytestrings as per the P4Runtime spec: https://p4lang.github.io/p4runtime/spec/main/P4Runtime-Spec.html#sec-bytestrings. That's in addition to the requirement that masked off bits (according to the prefix length) be set to 0. If this is not implemented properly, yes I think the code should be changed.

I believe the latter positioning of the key within 3 bytes would be consistent with how P4Runtime encodes exact and ternary match fields

All values should be encoded the same, regardless of the match type. But it sounds like the way we try to enforce the additional requirement (trailing zeros) for LPM matches is conflicting with using a valid bytestring encoding.

It makes sense that the same issue would exist in p4runtime-shell (encoding) and in PI (decoding). That could also explain why the issue went undetected. I wonder if the correct table entry ends up being added to bmv2 or not?

[jafingerhut]

So I have written a little test P4 program [1] with a 20-bit lpm key field, and a PTF test that exercises it using the P4Runtime API and p4runtime-shell, and I tried it with changes to p4runtime-shell from this PR, and a small change to PI that seems to correct a sanity check there, but when I run that test it seems that BMv2 has a fair amount of code in its match table implementation that currently assumes that an LPM key field's most significant bit is in the most significant bit of the most significant byte of the key.

This can be changed, but it seems to be far from a one-line change, and I don't have a proposed change that enables my test to pass yet.

[1] https://github.com/jafingerhut/p4-guide/tree/master/lpmtester/lpmwidth20bits

[dorakerman]

Hello and sorry for late response again

Thank you for the thorough testing!

You're correct, we are using a P4Runtime server implementation that was developed independently of the p4lang/PI repository.

Given that the current obstacle seems to involve a more significant refactoring within the BMv2, I wanted to ask, is there anything further I can contribute to this Pull Request at this stage to prepare it for merging? Please let me know if I can assist in anything :)

Thanks!

[jafingerhut]

Hello and sorry for late response again

Thank you for the thorough testing!

You're correct, we are using a P4Runtime server implementation that was developed independently of the p4lang/PI repository.

Given that the current obstacle seems to involve a more significant refactoring within the BMv2, I wanted to ask, is there anything further I can contribute to this Pull Request at this stage to prepare it for merging? Please let me know if I can assist in anything :)

Thanks!

Since your change only affects the behavior when an LPM lookup key is not a multiple of 8 bits wide, I think it would be reasonable to proceed with:

(a) merging this PR soon, and
(b) creating issues in the PI and behavioral-model repositories that there is a known bug for such table key fields, which until that issue has been corrected, one can work around it by using an LPM key field that is a multiple of 8 bits wide.

I can get (a) done soon, and also creating the issue for (b).

[jafingerhut]

I have created these issues to track making corresponding fixes to the PI and behavioral-model repositories:

• Wrong bitwise alignment expected for LPM keys when they are not multiples of 8 bits in size PI#625
• Wrong bitwise alignment expected for LPM keys when they are not multiples of 8 bits in size behavioral-model#1331

By merging in this PR for the p4runtime-shell repository before those issues are fixed, the only problem that will be introduced is that using p4runtime-shell in combination with BMv2+PI will fail to handle LPM key fields that are not multiples of 8 bits wide. By far the most common case is that LPM key fields are multiples of 8 bits wide, so while this might affect a few people, I think it would be a small fraction.

@antoninbas Any objections from you for merging this PR very soon, even if the above linked issues are not yet fixed? I suspect I'll have a fix for the PI and behavioral-model issues in less than a month.

[antoninbas]

@jafingerhut No objection from me, I'll trust whichever call you make on this

[jafingerhut]

Thanks, Antonin. Since I have created the relevant issues for the PI and behavioral-model repositories, I will go ahead and merge this PR.

[jafingerhut]

LGTM

[dorakerman]

@jafingerhut Hi Andy, can you please tell me if this fix will be introduced in a tag in the near future? Asking so I can change my testing accordingly :)

[jafingerhut]

@jafingerhut Hi Andy, can you please tell me if this fix will be introduced in a tag in the near future? Asking so I can change my testing accordingly :)

Version 0.0.6 has been released to PyPI, which includes the LPM mask fix: https://pypi.org/project/p4runtime-shell/

[dorakerman]

Thanks again!