Port table improvements

[PlagueCZ]

Motivation

• (primary) change struct dp_port *dp_port_get(port_id) (called inside many functions) from list traversal O(n) to direct array access O(1), which happened for every packet
• remove multiple calls to rte_eth_dev_socket_id() on the same port_id
• get rid of RTE_VERIFY(port_id < RTE_DIM(some_port_indexed_table)) that can crash
• put all some_global_table[DP_MAX_PORTS] into one place

Changes

This caused replacing most of uint16_t port_id function arguments to struct dp_port *port, which is the largest part of the changes across the codebase.

I created a table of ports and moved all vm_entry and internal_stats into struct dp_port since all these were also accessed by a port_id. This also enabled doing a DP_FOREACH_PORT to iterate VMs (that were previously traversing all possible 128 ports even when not in use).

In the end I inlined hot-functions (like dp_port_get(m->port_id) since it gets called multiple times for each packet traversal). This does require having extern _dp_variables in the header though.

To further optimize, there are now dp_get_port(m) and dp_get_dst_port(df) that do not check the table index, thus will never fail, based on the fact that m->port and df->nxt_hop are already checked once. If this is something unwanted, I can change it, though it really complicates the code with all the if (!port) return DROP_ etc.

I also dropped some of the wrappers (like dp_get_fwall_head(port_id)) and directly use port->vm.fwall_head because:

• port is already valid pointer (no need to check port_id index as before)
• it does not imply checking the return value for NULL
• array size for memcpy() can be checked by sizeof() now

Since I touched many dp_lpm_ functions anyway I noticed that there are many VM-related ones that are not LPM/routing related, so I separated those into dp_vm.c.

Also small changes like adding a pci_name to struct dp_port to prevent system calls in every listing of interfaces again and again; changing the logging macro to directly use dp_port, etc.

---

One thing that I changed and am not 100% sure about is the fact that previously some VNI functions, like dp_lpm_reset_route_tables(vni, socket_id) or dp_get_vni_route4_table(vni, socket_id) used the argument socket_id that was retrieved from port_id. But there already is a stored socket_id in struct vni_value, so I used that instead as that seems to be the desired use.

[guvenc]

@PlagueCZ
Thanks. This looks good to me so far. I just have two questions to the explanation part of the PR. I can not the see the place in the old code where struct dp_port *dp_port_get(port_id) was called for "each" packet ? It is called in GRPC handling, startup and during packet offloading rule installations (is that what you mean for each packet ?) but in the non-offloading code path, I can not seem to pinpoint the place where it is called for "each" packet.
Not that it would make this refactoring less valuable. I am probably overlooking the place and am curious to know where it is.

Second question is dp_lpm_reset_route_tables(vni, socket_id) seems to still use socket_id from the parameter although you tell in the explanation that it is not using it from the parameter anymore. The socketid changes are ok but just want to check the discrepancy.

[PlagueCZ]

The use of dp_port_get() is hidden in many functions where it calls directly a static inline struct dp_port *get_port(uint16_t port_id) that iterates all ports to find the right one.

On of such calls is for example dp_port_get_vf_attach_status() called in ipv4_lookup before passing the packet on. Then also dp_multipath_get_pf() calls dp_port_get_link_status() which then iterates ports again. There are other instances I think.

---

You are right about the dp_reset_vni_route_tables(), as I only changed it on the code path where I was changing the port_id to dp_port and I must have overlooked this usage. I'll push another commit

[byteocean]

it is true that the port status needs to be checked before packet processing continues when doing routing. however, if it is just about port checking for this particular case, would it be better and simpler if we use a bitmap or a simple array containing status instead of making this link status as a field in the port structure?

[PlagueCZ]

Tao's suggestion (using a table for statuses) would work perfectly well (and would actually be nicely localized), the only drawback I see is the requirement for array-bounds checking, which is one of the things this changes is trying to get rid of by verifying the m->port in CLS node.

[guvenc]

@PlagueCZ Thanks for the clarification.
Regarding bitmaps. I think the main motivation of this refactoring is eliminating the O(n) access and make it O(1) during packet processing and this is achieved by using the array approach. I would not further complicate by using a bitmap, this would probably have the advantage of using less memory but the access will still be O(1).

I think this is not smt introduced by this PR but I can not seem to find the place where the port->attached gets assigned to "false". If a port gets deallocated (port->allocated=false) It should actually also get port->attached=false
@byteocean Was this on purpose not assigning false to "attached" state of the port ? Or was this just forgotten ? Can you remember ?

[guvenc]

Thanks. Looks good to me. I will merge it and the open question regarding the port attached state will be handled by @byteocean in a different PR, if needed.