ironcore-dev / dpservice

DPDK based fast Dataplane / L3 router / SDN enabler, installable on compute nodes / SmartNICs
Apache License 2.0
7 stars 1 forks source link

Place iface_id as the first element in the struct for correct alignment on ARM #587

Closed guvenc closed 2 months ago

guvenc commented 2 months ago

On ARM architecture iface_id in "struct dpgrpc_vip" seems to start on odd memory address as it is not the first element of the struct where it is defined and the element before it, is packed.

This breaks the ifaced_id hash table lookup in create vip grpc call. So reorder the variables.

PlagueCZ commented 2 months ago

The change in itself looks OK, but I am kind of confused what the problem is. iface_id is a char array, thus a char pointer, which does not need to be aligned.

Is the problem the actual rte_hash_lookup_data() in dp_get_port_with_iface_id()? Because that needs to compute the hash and maybe it's trying to use some optimized code (for MMX/XMM for example) that needs aligned addresses.

In that case rte_aligned should help. Of course moving the element solves it too, but without any explanation, this can easily happen again, having the member/struct attribute there should prevent this in a possible change to the grpc structure.

guvenc commented 2 months ago

@PlagueCZ The problem is exactly as you described. I moved the element to beginning as the iface_id byte array is the first element in other structs in this header file. This seems to be a "convention" but only this struct had it at the second place so independent of the alignment problem, I would still put the iface_id to the first place of the struct to be consistent with the rest of the header file.

In that case, would you still see a value to put "__rte_aligned(4)" after the iface_id ? If so, actually to be consistent, we would need to put it after every iface_id variable in this headerfile.

PlagueCZ commented 2 months ago

@guvenc I agree on both accounts, i.e. it should be consistently on the first place, then if we were to add __rte_aligned(4) we should add it everywhere. It is the safest way for the future, so I would also do that.

Note that dpgrpc_capture_interface also has the iface_id in another place.

guvenc commented 2 months ago

588