[ovsp4rt] DPDK PrepareFdbRxVlanTableEntry port field
ffoulkes opened this issue · 0 comments
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.