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

[ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port field #683

Open
ffoulkes opened this issue Aug 31, 2024 · 0 comments
Open

[ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port field #683

ffoulkes opened this issue Aug 31, 2024 · 0 comments

Comments

@ffoulkes
Copy link
Contributor

ffoulkes commented Aug 31, 2024

Value semantics

The DPDK variant of PrepareFdbRxVlanTableEntry has unusual value semantics.

It uses the vln_info.vlan_id field as input, rather than a port-related field.

It also sets the value of the port action param to vln_info.vlan_id - 1.

Why does it do this?

Why is this logic in ovs-p4rt instead of being handled in OVS? I would have expected OVS to set a port_id (or equivalent) input field to the appropriate value, rather than conveying it in the vlan_id input field, and I would have expected OVS, not ovs-p4rt, to make the adjustment.

The preferred solution is to have OVS pass the value in a "port" field of some kind, and to have ovs-p4rt encode that value (without modification). The ES2K variant appears to use the rx_src_port input field for this purpose. Could the DPDK variant do the same?

If this is not possible, then the logic to encode the parameter value should be moved to a purpose-specific Encode function, and that function should be commented to explain what the hell is going on.

Field width

The DPDK variant of PrepareFdbRxVlanTableEntry encodes the port action param as a single byte.

The input field (vln_info.vlan_id) is uint32_t.

The action param (port) is bit<32> (PortId_t).

VLAN identifiers are bit<12>.

What is the actual width of the value? Is it 32 bits? If so, the action param should be encoded as four bytes.

If it's a smaller value (e.g. 16 bits) in a wider field, should it be encoded as two or three bytes?

If it really is an 8-bit value, there should be a comment in the code stating encoding the value as a single octet is correct. The disparity between the encoded byte and the widths of the input and output fields makes this a code smell.

See also: #689.

@ffoulkes ffoulkes changed the title [ovsp4rt] DPDK PrepareFdbRxVlanTableEntry has unusual value semantics [ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port_id action param Aug 31, 2024
@ffoulkes ffoulkes changed the title [ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port_id action param [ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port field Aug 31, 2024
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

1 participant