Re: [RFC 3/4] net/ixgbe: remove use of VLAs

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 5/23/2024 5:26 PM, Konstantin Ananyev wrote:
> From: Konstantin Ananyev <konstantin.ananyev@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.ananyev@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 */