On 5/23/2024 5:26 PM, Konstantin Ananyev wrote:
> From: Konstantin Ananyev <konstantin.anan...@huawei.com>
>
> 1) ../drivers/net/ixgbe/ixgbe_ethdev.c:3556:46: warning: variable length
> array used [-Wvla]
> 2) ../drivers/net/ixgbe/ixgbe_ethdev.c:3739:23: warning: variable length
> array used [-Wvla]
> 3) ../drivers/net/ixgbe/ixgbe_rxtx_vec_common.h:17:24: warning: variable
> length array used [-Wvla]
>
> For first two cases: in fact ixgbe_xstats_calc_num() always returns
> constant value, so it should be safe to replace that function invocation
> just with a macro that performs same calculations.
>
ack
> For case #3: reassemble_packets() is invoked only by
> ixgbe_recv_scattered_burst_vec().
> And in turn, ixgbe_recv_scattered_burst_vec() operates only on fixed
> max amount of packets per call: RTE_IXGBE_MAX_RX_BURST.
> So, it should be safe to replace VLA with fixed size array.
>
Ack
I see you already add assert, +1.
Do you think adding a comment that caller calls with max
RTE_IXGBE_MAX_RX_BURST helps?
>
> Signed-off-by: Konstantin Ananyev <konstantin.anan...@huawei.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 21 +++++++++++++--------
> drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 4 +++-
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index bbdc996f31..7843cd41d3 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -3432,11 +3432,16 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev)
> }
>
> /* This function calculates the number of xstats based on the current config
> */
> +
> +#define IXGBE_XSTATS_CALC_NUM \
> + (IXGBE_NB_HW_STATS + IXGBE_NB_MACSEC_STATS + \
> + (IXGBE_NB_RXQ_PRIO_STATS * IXGBE_NB_RXQ_PRIO_VALUES) + \
> + (IXGBE_NB_TXQ_PRIO_STATS * IXGBE_NB_TXQ_PRIO_VALUES))
> +
> static unsigned
> -ixgbe_xstats_calc_num(void) {
> - return IXGBE_NB_HW_STATS + IXGBE_NB_MACSEC_STATS +
> - (IXGBE_NB_RXQ_PRIO_STATS * IXGBE_NB_RXQ_PRIO_VALUES) +
> - (IXGBE_NB_TXQ_PRIO_STATS * IXGBE_NB_TXQ_PRIO_VALUES);
> +ixgbe_xstats_calc_num(void)
> +{
> + return IXGBE_XSTATS_CALC_NUM;
> }
>
> static int ixgbe_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> @@ -3552,8 +3557,8 @@ static int ixgbe_dev_xstats_get_names_by_id(
> }
>
> uint16_t i;
> - uint16_t size = ixgbe_xstats_calc_num();
> - struct rte_eth_xstat_name xstats_names_copy[size];
> + struct rte_eth_xstat_name xstats_names_copy[IXGBE_XSTATS_CALC_NUM];
> + const uint16_t size = RTE_DIM(xstats_names_copy);
>
> ixgbe_dev_xstats_get_names_by_id(dev, NULL, xstats_names_copy,
> size);
> @@ -3735,8 +3740,8 @@ ixgbe_dev_xstats_get_by_id(struct rte_eth_dev *dev,
> const uint64_t *ids,
> }
>
> uint16_t i;
> - uint16_t size = ixgbe_xstats_calc_num();
> - uint64_t values_copy[size];
> + uint64_t values_copy[IXGBE_XSTATS_CALC_NUM];
> + const uint16_t size = RTE_DIM(values_copy);
>
> ixgbe_dev_xstats_get_by_id(dev, NULL, values_copy, size);
>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> index a4d9ec9b08..c1cf0a581a 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> @@ -14,11 +14,13 @@ static inline uint16_t
> reassemble_packets(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_bufs,
> uint16_t nb_bufs, uint8_t *split_flags)
> {
> - struct rte_mbuf *pkts[nb_bufs]; /*finished pkts*/
> + struct rte_mbuf *pkts[RTE_IXGBE_MAX_RX_BURST]; /*finished pkts*/
> struct rte_mbuf *start = rxq->pkt_first_seg;
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned int pkt_idx, buf_idx;
>
> + RTE_ASSERT(nb_bufs <= RTE_DIM(pkts));
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end != NULL) {
> /* processing a split packet */
