From: fengchengwen <fengchengwen@huawei.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: <thomas@monjalon.net>, <ferruh.yigit@amd.com>, <dev@dpdk.org>,
<huangdengdui@huawei.com>, <mb@smartsharesystems.com>,
<roretzla@linux.microsoft.com>
Subject: Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
Date: Fri, 12 Apr 2024 16:16:54 +0800 [thread overview]
Message-ID: <6dfc109e-8b02-e8d4-c14f-03febf33839e@huawei.com> (raw)
In-Reply-To: <20240411080530.1f5030c6@hermes.local>
Hi Stephen,
On 2024/4/11 23:05, Stephen Hemminger wrote:
> On Thu, 11 Apr 2024 03:07:49 +0000
> Chengwen Feng <fengchengwen@huawei.com> 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: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> ---
>
> 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/20180108174514.14688-1-stephen@networkplumber.org/
https://inbox.dpdk.org/dev/20180116182558.6254-1-stephen@networkplumber.org/
>
> 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
>
> .
>
next prev parent reply other threads:[~2024-04-12 8:16 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 3:07 Chengwen Feng
2024-04-11 6:53 ` Morten Brørup
2024-04-11 6:58 ` Morten Brørup
2024-04-11 11:57 ` fengchengwen
2024-04-11 12:44 ` Morten Brørup
2024-04-11 12:04 ` [PATCH v3] " Chengwen Feng
2024-04-11 12:44 ` Morten Brørup
2024-04-12 3:27 ` fengchengwen
2024-04-12 7:24 ` Morten Brørup
2024-04-11 15:05 ` [PATCH] " Stephen Hemminger
2024-04-12 8:16 ` fengchengwen [this message]
2024-04-12 8:49 ` [PATCH v4] " Chengwen Feng
2024-04-13 8:04 ` [PATCH v5] " Chengwen Feng
2024-04-13 8:48 ` [PATCH v6] " Chengwen Feng
2024-04-15 13:15 ` Morten Brørup
2024-04-18 7:28 ` [PATCH v7] " Chengwen Feng
2024-04-19 15:15 ` Ferruh Yigit
2024-04-19 15:25 ` Ferruh Yigit
2024-04-22 6:42 ` fengchengwen
2024-04-22 6:38 ` [PATCH v8] " Chengwen Feng
2024-04-22 10:54 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6dfc109e-8b02-e8d4-c14f-03febf33839e@huawei.com \
--to=fengchengwen@huawei.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=huangdengdui@huawei.com \
--cc=mb@smartsharesystems.com \
--cc=roretzla@linux.microsoft.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).