RE: Strict aliasing problem with rte_eth_linkstatus_set()

["Morten Brørup" <mb@smartsharesystems.com>]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Wednesday, 10 April 2024 11.34
> 
> Hi All,
> 
> We have a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> we've done some
> research but haven't been able to figure out why. We'd like the community's
> help.
> 
> Environment:
> 1. Source: DPDK 23.11
> 2. GCC: 12.3.1 [1]
> 3. Compiled with target kunpeng SoC (ARM64)
> 4. Run on kunpeng SoC
> 
> 
> Problem & Debug:
> 1. We found the hns3 driver fails to update the link status. The corresponding
> function is
> hns3_update_linkstatus_and_event [2], and we found the rte_eth_linkstatus_set
> [3] always return zero.
> 2. After disassembly the hns3_update_linkstatus_and_event, and found
> rte_eth_linkstatus_set's
> rte_atomic_exchange_explicit return to xzr register (which is zero register):
>  1239fec:       3900f3e0        strb    w0, [sp, #60]
>  1239ff0:       9101a041        add     x1, x2, #0x68      ---x2 seem not the
> variable new_link
>  1239ff4:       f8ff8022        swpal   xzr, x2, [x1]      ---this instr
> corresponding rte_atomic_exchange_explicit,
>                                                               it will place
> the resut in xzr which always zero,
>                                                               and this will
> lead to rte_eth_linkstatus_set return 0.
>  1239ff8:       3940f3e0        ldrb    w0, [sp, #60]
>  1239ffc:       d3609c41        ubfx    x1, x2, #32, #8
>  123a000:       4a010000        eor     w0, w0, w1
>  123a004:       36100080        tbz     w0, #2, 123a014
> <hns3_update_linkstatus_and_event+0xd4>
> 3. We checked other "ret = rte_eth_linkstatus_set" calls, and can't find
> similar problem.
> 4. After reading a lot of documents, we preliminarily think that the problem
> is caused by -fstrict-aliasing
> (which was enabled default with O2 or O3), if compiled with -fno-strict-
> aliasing, then this problem don't
> exist. We guest this maybe strict-aliasing's bug which only happened in our
> function.
> 5. We also try to use union to avoid such aliasing in rte_eth_linkstatus_set,
> we changed the struct
> rte_eth_link define, and it works:
>   -__extension__
>   -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write
> */
>   -       uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
>   -       uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>   -       uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>   -       uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
>   +struct rte_eth_link { /**< aligned for atomic64 read/write */
>   +       union {
>   +               uint64_t val64;
>   +               struct {
>   +                       uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_
> */
>   +                       uint16_t link_duplex  : 1;  /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>   +                       uint16_t link_autoneg : 1;  /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
>   +                       uint16_t link_status  : 1;  /**<
> RTE_ETH_LINK_[DOWN/UP] */
>   +               };
>   +       };
>   };
> the corresponding rte_eth_linkstatus_set:
>   @@ -1674,18 +1674,13 @@ static inline int
>    rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>                          const struct rte_eth_link *new_link)
>    {
>   -       RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
>   -       union {
>   -               uint64_t val64;
>   -               struct rte_eth_link link;
>   -       } orig;
>   -
>   -       RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>   +       struct rte_eth_link old_link;
> 
>   -       orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
> *)new_link,
>   +       old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,
>   +                                       new_link->val64,
>                                           rte_memory_order_seq_cst);
> 
>   -       return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>   +       return (old_link.link_status == new_link->link_status) ? -1 : 0;
>    }
> 6. BTW: the linux kernel enabled "-fno-strict-aliasing" default, please see
> [4] for more.
> 

Thank you for the detailed analysis.
Looking at the GCC documentation for -fstrict-aliasing supports your conclusion.

Just out of curiosity:
Does building with -Wstrict-aliasing=3 or -Wstrict-aliasing=1 provide any useful information?
And are the any differences in the strict-aliasing warnings without/with your fix?

> 
> Last： We think there are two ways to solve this problem.
> 1. Add the compilation option '-fno-strict-aliasing' for hold DPDK project.
> 2. Use union to avoid such aliasing in rte_eth_linkstatus_set (please see
> above).
> PS: We prefer first way.
> 
> Hope for more discuess.
> 
> Thanks

Unfortunately, DPDK uses a lot of type casting where other methods would be formally more correct.
I fear that you only found one of potentially many more bugs like this.
Fixing this generally would be great, but probably not realistic.

I prefer correctness over performance, and thus am in favor of adding -fno-strict-aliasing project wide.

Performance can be improved by adding more hints and attributes to functions and parameters, e.g. rte_memcpy() could be:
__attribute__((access(write_only, 1, 3), access(read_only, 2, 3)))
static void *
rte_memcpy(void * restrict dst, const void * restrict src, size_t n);

Instead of just:
static void *
rte_memcpy(void * dst, const void * src, size_t n);


> 
> 
> [1] https://developer.arm.com/downloads/-/arm-gnu-toolchain-downloads/12-3-
> rel1
> [2] void
> hns3_update_linkstatus_and_event(struct hns3_hw *hw, bool query)
> {
> 	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
> 	struct rte_eth_link new_link;
> 	int ret;
> 
> 	if (query)
> 		hns3_update_port_link_info(dev);
> 
> 	memset(&new_link, 0, sizeof(new_link));
> 	hns3_setup_linkstatus(dev, &new_link);
> 
> 	ret = rte_eth_linkstatus_set(dev, &new_link);
> 	if (ret == 0 && dev->data->dev_conf.intr_conf.lsc != 0)
> 		hns3_start_report_lse(dev);
> }
> [3] static inline int
> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> 		       const struct rte_eth_link *new_link)
> {
> 	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> 	union {
> 		uint64_t val64;
> 		struct rte_eth_link link;
> 	} orig;
> 
> 	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> 
> 	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
> *)new_link,
> 					rte_memory_order_seq_cst);
> 
> 	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> }
> [4] https://lore.kernel.org/all/b3itcd$2bi$1@penguin.transmeta.com/