Hi Stephen,
On 2024/4/11 23:05, Stephen Hemminger wrote:
> On Thu, 11 Apr 2024 03:07:49 +0000
> Chengwen Feng <[email protected]> wrote:
>
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: [email protected]
>>
>> Signed-off-by: Chengwen Feng <[email protected]>
>> Signed-off-by: Dengdui Huang <[email protected]>
>> ---
>
> The patch to use union is correct.
> Examining the link status fuller raises a couple of pre-existing issues.
>
> 1. Why is this an inline function, there is no way this is in the
> fast path of any driver or application?
Nice catch.
The rte_eth_linkstatus_set/get was first introduced by commit "b77d21cc23
ethdev: add link status get/set helper functions".
which were inline function.
But I check this patchset [1], and found in v1~v3 is was impl without static
inline, and the v4 just with static inline. and
I can't get the change reason. @Stephen could you recall something about this?
Below is what I try to find the reason from source code:
1, Why rte_eth_linkstatus_get is inline, I think maybe due:
a, memif driver invoke rte_eth_link_get() in Rx/Tx path
(eth_memif_rx/eth_memif_rx_zc/eth_memif_tx/eth_memif_tx_zc)
b, before the above commit, rte_eth_link_get() invoke
rte_eth_dev_atomic_read_link_status() which was static inline.
from above, I think it mainly due to performance reason, so keep the
original impl.
2, Why rte_eth_linkstatus_set is inline, I check the following patch:
5042dde07d net/enic: use link status helper functions
2b4ab4223d net/octeontx: use link status helper functions
cc92eb9a97 net/szedata2: use link status helper functions
8e14dc285a net/thunderx: use link status helper functions
e324523c6c net/liquidio: use link status helper functions
e66b0fd123 net/i40e: use link status helper functions
4abe903e50 net/sfc: use link status helper functions
faadebad81 net/ixgbe: use link status helper functions
80ba61115e net/e1000: use link status helper functions
aab28ea2bc net/nfp: use link status helper functions
7e2eb5f0d2 net/dpaa2: use link status helper functions
13086a8f50 net/vmxnet3: use link status helper functions
717b2e8eae net/virtio: use link status helper functions
almost all pmd's set link function is static inline, I also check, and found
only sfc driver will invoke it in
Tx path:
sfc_efx_recv_pkts->sfc_ev_qpoll->efx_ev_qpoll->eevo_qpoll->siena_ef10_ev_qpoll->rhead_ev_mcdi->ef10_ev_mcdi->sfc_ev_link_change->rte_eth_linkstatus_set
[1]
https://inbox.dpdk.org/dev/[email protected]/
https://inbox.dpdk.org/dev/[email protected]/
>
> 2. Why is it marked sequential consistent and not relaxed? How could there be
> a visible
> relationship between link status and other variables. Drivers would
> not be using the link status state as internal variable.
The original was impl by rte_atomic64_cmpset():
1, In powerpc platform, it was impl as:
static inline int
rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
{
return rte_atomic_compare_exchange_strong_explicit(dst, &exp, src,
rte_memory_order_acquire,
rte_memory_order_acquire) ? 1 : 0;
}
2, In ARM64 platform, it was impl as:
static inline int
rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
{
return __sync_bool_compare_and_swap(dst, exp, src);
}
The __sync_bool_compare_and_swap will compiled with a dmb.
So there's no problem from the point of view of replacement (just with the
memory barrier).
I also think there are no need with sequential consistent, because this atomic
mainly to solve
the concurrent problem of rte_eth_link_get and driver lsc interrupt setting.
BTW: the above two problem is OK, but I think we could tackle with another
commit (NOT this).
Thanks
>
> .
>
