[RFC 0/6] core libs: remove VLA warnings

[RFC 0/6] core libs: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

This RFC is submitted as part of ongoing VLA removal discussion:
https://patchwork.dpdk.org/project/dpdk/list/?series=31680
The main aim is to explore amount of code changes that would be required for that,
plus explore alternate approaches instead of simple replacing VLAs with alloca() calls.
It touches core libraries only:
always_enable = [
        'cmdline',
        'eal',
        'ethdev',
        'hash',
        'kvargs',
        'log',
        'mbuf',
        'mempool',
        'meter',
        'net',
        'pci',
        'rcu',
        'ring',
        'stack',
        'telemetry',
 ]
For just these libs, we have reaonably small number of VLA warnings:
        1 ../lib/eal/common
	2 ../lib/eal/linux
 	2 ../lib/rcu
	3 ../lib/hash
	4 ../lib/ethdev

The series addresses them one by one, on individual basis.
Disclaimer: this is just an RFC, so some approaches might be questionable
(eal_interrupts).
Others (rcu) shall be treated as API change, so can be applied only for 24.11.
The main goal here is to start discussion and find the best possible approach.

Just as FYI, below is the number of VLA warnings per module for all libs and drivers on my machine
for linux 'default' build with gcc:

      1 ../drivers/crypto/cnxk
      1 ../drivers/crypto/octeontx
      1 ../drivers/crypto/scheduler
      1 ../drivers/net/dpaa
      1 ../drivers/net/failsafe
      1 ../drivers/net/gve
      1 ../drivers/net/i40e
      1 ../drivers/net/ice
      1 ../drivers/net/netvsc
      1 ../drivers/net/sfc
      1 ../lib/eventdev
      1 ../lib/latencystats
      1 ../lib/power
      2 ../drivers/common/mlx5
      2 ../drivers/crypto/ccp
      2 ../drivers/crypto/openssl
      2 ../drivers/net/nfp
      2 ../drivers/net/octeontx
      2 ../drivers/net/softnic
      2 ../drivers/net/vdev_netvsc
      2 ../lib/acl
      2 ../lib/dispatcher
      2 ../lib/eal/linux
      2 ../lib/gro
      2 ../lib/pdump
      2 ../lib/rcu
      3 ../drivers/common/cnxk
      3 ../drivers/common/idpf
      3 ../drivers/crypto/qat
      3 ../drivers/event/dsw
      3 ../drivers/net/bnxt
      3 ../drivers/net/cxgbe
      3 ../drivers/net/ixgbe
      3 ../lib/hash
      3 ../lib/pdcp
      3 ../lib/table
      4 ../drivers/net/bonding
      4 ../drivers/net/cnxk
      4 ../drivers/net/hns3
      4 ../drivers/net/tap
      4 ../lib/ethdev
      5 ../drivers/event/opdl
      5 ../drivers/net/dpaa2
      5 ../drivers/vdpa/mlx5
      5 ../lib/member
      6 ../drivers/event/cnxk
      6 ../lib/bpf
      6 ../lib/vhost
      7 ../drivers/common/mlx5/linux
      8 ../drivers/net/virtio
     10 ../drivers/net/mlx4
     11 ../drivers/net/mlx5
     13 ../drivers/event/sw
     17 ../drivers/net/mlx5/linux
     22 ../lib/lpm
     61 ../lib/ipsec

Konstantin Ananyev (6):
  eal/linux: remove VLA warnings
  eal/common: remove VLA warnings
  ethdev: remove VLA warnings
  hash: remove VLA warnings
  hash/thash: remove VLA warnings
  rcu: remove VLA warnings

 lib/eal/common/eal_common_proc.c |   5 +-
 lib/eal/linux/eal_interrupts.c   |  59 ++++++++--
 lib/ethdev/rte_ethdev.c          | 183 +++++++++++++++++++------------
 lib/hash/rte_cuckoo_hash.c       |   4 +-
 lib/hash/rte_thash.c             |   2 +-
 lib/hash/rte_thash.h             |   8 ++
 lib/rcu/rte_rcu_qsbr.c           |   7 +-
 lib/rcu/rte_rcu_qsbr.h           |   5 +
 8 files changed, 188 insertions(+), 85 deletions(-)

-- 
2.35.3

[RFC 1/6] eal/linux: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ./lib/eal/linux/eal_interrupts.c:1073:16: warning: ISO C90 forbids variable length array ‘events’ [-Wvla]

eal_intr_handle_interrupts() is called by eal_intr_thread_main()
so it seems ok to simply alloc space for events from heap and reuse the
same buffer through the life of the thread.

2) ./lib/eal/linux/eal_interrupts.c:1319:16: warning: ISO C90 forbids variable length array ‘evs’ [-Wvla]

make eal_epoll_wait() to use fixed size array and use it though multiple
iterations to preocess upt to @maxevents events.
Note that techically it is not one to one raplacement, as here we might
reduce number of events returned by first call to epoll_wait(..., timeout);

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/eal/linux/eal_interrupts.c | 59 ++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/lib/eal/linux/eal_interrupts.c b/lib/eal/linux/eal_interrupts.c
index 6436f796eb..58e3b8e12c 100644
--- a/lib/eal/linux/eal_interrupts.c
+++ b/lib/eal/linux/eal_interrupts.c
@@ -34,6 +34,8 @@
 #define EAL_INTR_EPOLL_WAIT_FOREVER (-1)
 #define NB_OTHER_INTR               1
 
+#define MAX_ITER_EVNUM	RTE_EVENT_ETH_INTR_RING_SIZE
+
 static RTE_DEFINE_PER_LCORE(int, _epfd) = -1; /**< epoll fd per thread */
 
 /**
@@ -1068,9 +1070,9 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
  *  void
  */
 static void
-eal_intr_handle_interrupts(int pfd, unsigned totalfds)
+eal_intr_handle_interrupts(int pfd, struct epoll_event events[],
+	unsigned totalfds)
 {
-	struct epoll_event events[totalfds];
 	int nfds = 0;
 
 	for(;;) {
@@ -1106,6 +1108,12 @@ eal_intr_handle_interrupts(int pfd, unsigned totalfds)
 static __rte_noreturn uint32_t
 eal_intr_thread_main(__rte_unused void *arg)
 {
+	uint32_t n, nb_event;
+	struct epoll_event *events, *p;
+
+	nb_event = 0;
+	events = NULL;
+
 	/* host thread, never break out */
 	for (;;) {
 		/* build up the epoll fd with all descriptors we are to
@@ -1159,8 +1167,23 @@ eal_intr_thread_main(__rte_unused void *arg)
 				numfds++;
 		}
 		rte_spinlock_unlock(&intr_lock);
+
+		/* alloc space for events, when necessary */
+		if (numfds > nb_event) {
+			n = numfds + MAX_ITER_EVNUM;
+			p = realloc(events, n * sizeof(events[0]));
+			if (p == NULL) {
+				EAL_LOG(ERR, "failed to allocate %u events",
+					numfds);
+				numfds = nb_event;
+			} else {
+				nb_event = n;
+				events = p;
+			}
+		}
+
 		/* serve the interrupt */
-		eal_intr_handle_interrupts(pfd, numfds);
+		eal_intr_handle_interrupts(pfd, events, numfds);
 
 		/**
 		 * when we return, we need to rebuild the
@@ -1168,6 +1191,8 @@ eal_intr_thread_main(__rte_unused void *arg)
 		 */
 		close(pfd);
 	}
+
+	free(events);
 }
 
 int
@@ -1316,8 +1341,9 @@ static int
 eal_epoll_wait(int epfd, struct rte_epoll_event *events,
 	       int maxevents, int timeout, bool interruptible)
 {
-	struct epoll_event evs[maxevents];
 	int rc;
+	uint32_t i, k, n, num;
+	struct epoll_event evs[MAX_ITER_EVNUM];
 
 	if (!events) {
 		EAL_LOG(ERR, "rte_epoll_event can't be NULL");
@@ -1328,12 +1354,31 @@ eal_epoll_wait(int epfd, struct rte_epoll_event *events,
 	if (epfd == RTE_EPOLL_PER_THREAD)
 		epfd = rte_intr_tls_epfd();
 
+	num = maxevents;
+	n = RTE_MIN(RTE_DIM(evs), num);
+
+	/* Process events in chunks of MAX_ITER_EVNUM */
+
 	while (1) {
-		rc = epoll_wait(epfd, evs, maxevents, timeout);
+		rc = epoll_wait(epfd, evs, n, timeout);
 		if (likely(rc > 0)) {
+
 			/* epoll_wait has at least one fd ready to read */
-			rc = eal_epoll_process_event(evs, rc, events);
-			break;
+			for (i = 0, k = 0; rc > 0;) {
+				k += rc;
+				rc = eal_epoll_process_event(evs, rc,
+						events + i);
+				i += rc;
+
+				/*
+				 * try to read more events that are already
+				 * available (up to maxevents in total).
+				 */
+				n = RTE_MIN(RTE_DIM(evs), num - k);
+				rc = (n == 0) ? 0 : epoll_wait(epfd, evs, n, 0);
+			}
+			return i;
+
 		} else if (rc < 0) {
 			if (errno == EINTR) {
 				if (interruptible)
-- 
2.35.3

RE: [RFC 1/6] eal/linux: remove VLA warnings

[Morten Brørup]

> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 
> 1) ./lib/eal/linux/eal_interrupts.c:1073:16: warning: ISO C90 forbids variable
> length array ‘events’ [-Wvla]
> 
> eal_intr_handle_interrupts() is called by eal_intr_thread_main()
> so it seems ok to simply alloc space for events from heap and reuse the
> same buffer through the life of the thread.
> 
> 2) ./lib/eal/linux/eal_interrupts.c:1319:16: warning: ISO C90 forbids variable
> length array ‘evs’ [-Wvla]
> 
> make eal_epoll_wait() to use fixed size array and use it though multiple
> iterations to preocess upt to @maxevents events.
> Note that techically it is not one to one raplacement, as here we might
> reduce number of events returned by first call to epoll_wait(..., timeout);
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

[RFC 2/6] eal/common: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ../lib/eal/common/eal_common_proc.c:695:15: warning: variable length array used [-Wvla]
        char control[CMSG_SPACE(fd_size)];
                     ^~~~~~~~~~~~~~~~~~~

As msg->num_fds should not exceed RTE_MP_MAX_FD_NUM, replaced
it with fixed size array.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/eal/common/eal_common_proc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index d24093937c..f21ce60a08 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -692,7 +692,8 @@ send_msg(const char *dst_path, struct rte_mp_msg *msg, int type)
 	struct sockaddr_un dst;
 	struct mp_msg_internal m;
 	int fd_size = msg->num_fds * sizeof(int);
-	char control[CMSG_SPACE(fd_size)];
+	int32_t control_sz = CMSG_SPACE(fd_size);
+	char control[CMSG_SPACE(sizeof(msg->fds))];
 
 	m.type = type;
 	memcpy(&m.msg, msg, sizeof(*msg));
@@ -712,7 +713,7 @@ send_msg(const char *dst_path, struct rte_mp_msg *msg, int type)
 	msgh.msg_iov = &iov;
 	msgh.msg_iovlen = 1;
 	msgh.msg_control = control;
-	msgh.msg_controllen = sizeof(control);
+	msgh.msg_controllen = control_sz;
 
 	cmsg = CMSG_FIRSTHDR(&msgh);
 	cmsg->cmsg_len = CMSG_LEN(fd_size);
-- 
2.35.3

RE: [RFC 2/6] eal/common: remove VLA warnings

[Morten Brørup]

> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> Sent: Thursday, 18 April 2024 12.33
> 
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 
> 1) ../lib/eal/common/eal_common_proc.c:695:15: warning: variable length array
> used [-Wvla]
>         char control[CMSG_SPACE(fd_size)];
>                      ^~~~~~~~~~~~~~~~~~~
> 
> As msg->num_fds should not exceed RTE_MP_MAX_FD_NUM, replaced
> it with fixed size array.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

[RFC 3/6] ethdev: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ./lib/ethdev/rte_ethdev.c:3244:16: warning: ISO C90 forbids variable length array ‘xstats_names’ [-Wvla]
2) ./lib/ethdev/rte_ethdev.c:3345:17: warning: ISO C90 forbids variable length array ‘ids_copy’ [-Wvla]
3) ./lib/ethdev/rte_ethdev.c:3538:16: warning: ISO C90 forbids variable length array ‘xstats’ [-Wvla]
4) ./lib/ethdev/rte_ethdev.c:3554:17: warning: ISO C90 forbids variable length array ‘ids_copy’ [-Wvla]

For 1) and 3) - just replaced VLA with arrays allocated from heap.
As I understand xstats extraction belongs to control-path, so extra
calloc/free is jopefully acceptable.
Also ethdev xstats already doing that within
rte_eth_xstats_get_id_by_name().
For 2) and 4) changed the code to use fixed size array and call
appropriate devops function several times, if needed.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 183 +++++++++++++++++++++++++---------------
 1 file changed, 113 insertions(+), 70 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f1c658f49e..e462f3de6f 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -36,6 +36,8 @@
 #include "ethdev_trace.h"
 #include "sff_telemetry.h"
 
+#define ETH_XSTATS_ITER_NUM	0x100
+
 struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
 
 /* public fast-path API */
@@ -3215,7 +3217,8 @@ int
 rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 		uint64_t *id)
 {
-	int cnt_xstats, idx_xstat;
+	int cnt_xstats, idx_xstat, rc;
+	struct rte_eth_xstat_name *xstats_names;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
@@ -3241,26 +3244,33 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name,
 	}
 
 	/* Get id-name lookup table */
-	struct rte_eth_xstat_name xstats_names[cnt_xstats];
+	xstats_names = calloc(cnt_xstats, sizeof(xstats_names[0]));
+	if (xstats_names == NULL) {
+		RTE_ETHDEV_LOG_LINE(ERR, "Can't allocate memory");
+		return -ENOMEM;
+	}
 
 	if (cnt_xstats != rte_eth_xstats_get_names_by_id(
 			port_id, xstats_names, cnt_xstats, NULL)) {
 		RTE_ETHDEV_LOG_LINE(ERR, "Cannot get xstats lookup");
+		free(xstats_names);
 		return -1;
 	}
 
+	rc = -EINVAL;
 	for (idx_xstat = 0; idx_xstat < cnt_xstats; idx_xstat++) {
 		if (!strcmp(xstats_names[idx_xstat].name, xstat_name)) {
 			*id = idx_xstat;
 
 			rte_eth_trace_xstats_get_id_by_name(port_id,
 							    xstat_name, *id);
-
-			return 0;
+			rc = 0;
+			break;
 		};
 	}
 
-	return -EINVAL;
+	free(xstats_names);
+	return rc;
 }
 
 /* retrieve basic stats names */
@@ -3306,6 +3316,38 @@ eth_basic_stats_get_names(struct rte_eth_dev *dev,
 	return cnt_used_entries;
 }
 
+static int
+eth_xstats_get_by_name_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
+	struct rte_eth_xstat_name *xstats_names, uint32_t size,
+	uint32_t basic_count)
+{
+	int32_t rc;
+	uint32_t i, k, m, n;
+	uint64_t ids_copy[ETH_XSTATS_ITER_NUM];
+
+	m = 0;
+	for (n = 0; n != size; n += k) {
+
+		k = RTE_MIN(size - n, RTE_DIM(ids_copy));
+
+		/*
+		 * Convert ids to xstats ids that PMD knows.
+		 * ids known by user are basic + extended stats.
+		 */
+		for (i = 0; i < k; i++)
+			ids_copy[i] = ids[n + i] - basic_count;
+
+		rc = (*dev->dev_ops->xstats_get_names_by_id)(dev, ids_copy,
+					xstats_names + m, k);
+		if (rc < 0)
+			return rc;
+		m += rc;
+	}
+
+	return m;
+}
+
+
 /* retrieve ethdev extended statistics names */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -3313,9 +3355,8 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	uint64_t *ids)
 {
 	struct rte_eth_xstat_name *xstats_names_copy;
-	unsigned int no_basic_stat_requested = 1;
-	unsigned int no_ext_stat_requested = 1;
 	unsigned int expected_entries;
+	unsigned int nb_basic_stats;
 	unsigned int basic_count;
 	struct rte_eth_dev *dev;
 	unsigned int i;
@@ -3341,27 +3382,18 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 	if (ids && !xstats_names)
 		return -EINVAL;
 
-	if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size > 0) {
-		uint64_t ids_copy[size];
-
-		for (i = 0; i < size; i++) {
-			if (ids[i] < basic_count) {
-				no_basic_stat_requested = 0;
-				break;
-			}
-
-			/*
-			 * Convert ids to xstats ids that PMD knows.
-			 * ids known by user are basic + extended stats.
-			 */
-			ids_copy[i] = ids[i] - basic_count;
-		}
-
-		if (no_basic_stat_requested)
-			return (*dev->dev_ops->xstats_get_names_by_id)(dev,
-					ids_copy, xstats_names, size);
+	nb_basic_stats = 0;
+	if (ids != NULL) {
+		for (i = 0; i < size; i++)
+			nb_basic_stats += (ids[i] < basic_count);
 	}
 
+	/* no baisc stats requested, devops function provided */
+	if (nb_basic_stats == 0 && ids != NULL && size != 0 &&
+			dev->dev_ops->xstats_get_names_by_id != NULL)
+		return eth_xstats_get_by_name_by_id(dev, ids, xstats_names,
+				size, basic_count);
+
 	/* Retrieve all stats */
 	if (!ids) {
 		int num_stats = rte_eth_xstats_get_names(port_id, xstats_names,
@@ -3380,17 +3412,8 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
 		return -ENOMEM;
 	}
 
-	if (ids) {
-		for (i = 0; i < size; i++) {
-			if (ids[i] >= basic_count) {
-				no_ext_stat_requested = 0;
-				break;
-			}
-		}
-	}
-
 	/* Fill xstats_names_copy structure */
-	if (ids && no_ext_stat_requested) {
+	if (ids && nb_basic_stats == size) {
 		eth_basic_stats_get_names(dev, xstats_names_copy);
 	} else {
 		ret = rte_eth_xstats_get_names(port_id, xstats_names_copy,
@@ -3514,17 +3537,47 @@ eth_basic_stats_get(uint16_t port_id, struct rte_eth_xstat *xstats)
 	return count;
 }
 
+static int
+eth_xtats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
+	uint64_t *values, uint32_t size, uint32_t basic_count)
+{
+	int32_t rc;
+	uint32_t i, k, m, n;
+	uint64_t ids_copy[ETH_XSTATS_ITER_NUM];
+
+	m = 0;
+	for (n = 0; n != size; n += k) {
+
+		k = RTE_MIN(size - n, RTE_DIM(ids_copy));
+
+		/*
+		 * Convert ids to xstats ids that PMD knows.
+		 * ids known by user are basic + extended stats.
+		 */
+		for (i = 0; i < k; i++)
+			ids_copy[i] = ids[n + i] - basic_count;
+
+		rc = (*dev->dev_ops->xstats_get_by_id)(dev, ids_copy,
+					values + m, k);
+		if (rc < 0)
+			return rc;
+		m += rc;
+	}
+
+	return m;
+}
+
 /* retrieve ethdev extended statistics */
 int
 rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			 uint64_t *values, unsigned int size)
 {
-	unsigned int no_basic_stat_requested = 1;
-	unsigned int no_ext_stat_requested = 1;
+	unsigned int nb_basic_stats;
 	unsigned int num_xstats_filled;
 	unsigned int basic_count;
 	uint16_t expected_entries;
 	struct rte_eth_dev *dev;
+	struct rte_eth_xstat *xstats;
 	unsigned int i;
 	int ret;
 
@@ -3535,7 +3588,6 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	if (ret < 0)
 		return ret;
 	expected_entries = (uint16_t)ret;
-	struct rte_eth_xstat xstats[expected_entries];
 	basic_count = eth_dev_get_xstats_basic_count(dev);
 
 	/* Return max number of stats if no ids given */
@@ -3549,51 +3601,41 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	if (ids && !values)
 		return -EINVAL;
 
-	if (ids && dev->dev_ops->xstats_get_by_id != NULL && size) {
-		unsigned int basic_count = eth_dev_get_xstats_basic_count(dev);
-		uint64_t ids_copy[size];
-
-		for (i = 0; i < size; i++) {
-			if (ids[i] < basic_count) {
-				no_basic_stat_requested = 0;
-				break;
-			}
-
-			/*
-			 * Convert ids to xstats ids that PMD knows.
-			 * ids known by user are basic + extended stats.
-			 */
-			ids_copy[i] = ids[i] - basic_count;
-		}
-
-		if (no_basic_stat_requested)
-			return (*dev->dev_ops->xstats_get_by_id)(dev, ids_copy,
-					values, size);
+	nb_basic_stats = 0;
+	if (ids != NULL) {
+		for (i = 0; i < size; i++)
+			nb_basic_stats += (ids[i] < basic_count);
 	}
 
-	if (ids) {
-		for (i = 0; i < size; i++) {
-			if (ids[i] >= basic_count) {
-				no_ext_stat_requested = 0;
-				break;
-			}
-		}
+	/* no baisc stats requested, devops function provided */
+	if (nb_basic_stats == 0 && ids != NULL && size != 0 &&
+			dev->dev_ops->xstats_get_by_id != NULL)
+		return eth_xtats_get_by_id(dev, ids, values, size, basic_count);
+
+	xstats = calloc(expected_entries, sizeof(xstats[0]));
+	if (xstats == NULL) {
+		RTE_ETHDEV_LOG_LINE(ERR, "Can't allocate memory");
+		return -ENOMEM;
 	}
 
 	/* Fill the xstats structure */
-	if (ids && no_ext_stat_requested)
+	if (ids && nb_basic_stats == size)
 		ret = eth_basic_stats_get(port_id, xstats);
 	else
 		ret = rte_eth_xstats_get(port_id, xstats, expected_entries);
 
-	if (ret < 0)
+	if (ret < 0) {
+		free(xstats);
 		return ret;
+	}
 	num_xstats_filled = (unsigned int)ret;
 
 	/* Return all stats */
 	if (!ids) {
 		for (i = 0; i < num_xstats_filled; i++)
 			values[i] = xstats[i].value;
+
+		free(xstats);
 		return expected_entries;
 	}
 
@@ -3601,14 +3643,15 @@ rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 	for (i = 0; i < size; i++) {
 		if (ids[i] >= expected_entries) {
 			RTE_ETHDEV_LOG_LINE(ERR, "Id value isn't valid");
-			return -1;
+			break;
 		}
 		values[i] = xstats[ids[i]].value;
 	}
 
 	rte_eth_trace_xstats_get_by_id(port_id, ids, values, size);
 
-	return size;
+	free(xstats);
+	return (i == size) ? (int32_t)size : -1;
 }
 
 int
-- 
2.35.3

RE: [RFC 3/6] ethdev: remove VLA warnings

[Morten Brørup]

> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> Sent: Thursday, 18 April 2024 12.33
> 
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 
> 1) ./lib/ethdev/rte_ethdev.c:3244:16: warning: ISO C90 forbids variable length
> array ‘xstats_names’ [-Wvla]
> 2) ./lib/ethdev/rte_ethdev.c:3345:17: warning: ISO C90 forbids variable length
> array ‘ids_copy’ [-Wvla]
> 3) ./lib/ethdev/rte_ethdev.c:3538:16: warning: ISO C90 forbids variable length
> array ‘xstats’ [-Wvla]
> 4) ./lib/ethdev/rte_ethdev.c:3554:17: warning: ISO C90 forbids variable length
> array ‘ids_copy’ [-Wvla]
> 
> For 1) and 3) - just replaced VLA with arrays allocated from heap.
> As I understand xstats extraction belongs to control-path, so extra
> calloc/free is jopefully acceptable.
> Also ethdev xstats already doing that within
> rte_eth_xstats_get_id_by_name().

Getting names and getting values are two different things.
I would slightly prefer alloca() as VLA replacement for "xstats".

> For 2) and 4) changed the code to use fixed size array and call
> appropriate devops function several times, if needed.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---

With or without suggested change...
Acked-by: Morten Brørup <mb@smartsharesystems.com>

Thank you very much for your work on getting rid of VLAs, Konstantin!

[RFC 4/6] hash: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ./lib/hash/rte_cuckoo_hash.c:2362:9: warning: ISO C90 forbids variable length array ‘positions’ [-Wvla]
2) ../lib/hash/rte_cuckoo_hash.c:2478:9: warning: ISO C90 forbids variable length array ‘positions’ [-Wvla]

Both rte_hash_lookup_bulk_data() and
rte_hash_lookup_with_hash_bulk_data() expect
@num_keys <= RTE_HASH_LOOKUP_BULK_MAX.
So, for both cases it should be safe to replace VLA with fixed size
array.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/hash/rte_cuckoo_hash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
index 9cf94645f6..82b74bda18 100644
--- a/lib/hash/rte_cuckoo_hash.c
+++ b/lib/hash/rte_cuckoo_hash.c
@@ -2359,7 +2359,7 @@ rte_hash_lookup_bulk_data(const struct rte_hash *h, const void **keys,
 			(num_keys > RTE_HASH_LOOKUP_BULK_MAX) ||
 			(hit_mask == NULL)), -EINVAL);
 
-	int32_t positions[num_keys];
+	int32_t positions[RTE_HASH_LOOKUP_BULK_MAX];
 
 	__rte_hash_lookup_bulk(h, keys, num_keys, positions, hit_mask, data);
 
@@ -2475,7 +2475,7 @@ rte_hash_lookup_with_hash_bulk_data(const struct rte_hash *h,
 			(num_keys > RTE_HASH_LOOKUP_BULK_MAX) ||
 			(hit_mask == NULL)), -EINVAL);
 
-	int32_t positions[num_keys];
+	int32_t positions[RTE_HASH_LOOKUP_BULK_MAX];
 
 	__rte_hash_lookup_with_hash_bulk(h, keys, sig, num_keys,
 			positions, hit_mask, data);
-- 
2.35.3

RE: [RFC 4/6] hash: remove VLA warnings

[Morten Brørup]

> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 
> 1) ./lib/hash/rte_cuckoo_hash.c:2362:9: warning: ISO C90 forbids variable
> length array ‘positions’ [-Wvla]
> 2) ../lib/hash/rte_cuckoo_hash.c:2478:9: warning: ISO C90 forbids variable
> length array ‘positions’ [-Wvla]
> 
> Both rte_hash_lookup_bulk_data() and
> rte_hash_lookup_with_hash_bulk_data() expect
> @num_keys <= RTE_HASH_LOOKUP_BULK_MAX.
> So, for both cases it should be safe to replace VLA with fixed size
> array.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

[RFC 5/6] hash/thash: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ./lib/hash/rte_thash.c:774:9: warning: ISO C90 forbids variable length array ‘tmp_tuple’ [-Wvla]

From my understanding, tuple size here should never exceed
sizeof(union rte_thash_tuple), so it should be safe to replace VLA with
fixed size array.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/hash/rte_thash.c | 2 +-
 lib/hash/rte_thash.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/hash/rte_thash.c b/lib/hash/rte_thash.c
index 68f653fa3d..e28d423172 100644
--- a/lib/hash/rte_thash.c
+++ b/lib/hash/rte_thash.c
@@ -771,7 +771,7 @@ rte_thash_adjust_tuple(struct rte_thash_ctx *ctx,
 	uint32_t desired_value,	unsigned int attempts,
 	rte_thash_check_tuple_t fn, void *userdata)
 {
-	uint32_t tmp_tuple[tuple_len / sizeof(uint32_t)];
+	uint32_t tmp_tuple[RTE_THASH_MAX_L4_LEN];
 	unsigned int i, j, ret = 0;
 	uint32_t hash, adj_bits;
 	const uint8_t *hash_key;
diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
index 30b657e67a..322fe3af66 100644
--- a/lib/hash/rte_thash.h
+++ b/lib/hash/rte_thash.h
@@ -108,6 +108,14 @@ union rte_thash_tuple {
 	struct rte_ipv6_tuple	v6;
 };
 
+/**
+ * maximum length in dwords of input tuple to
+ * calculate hash of ipv(4|6) header +
+ * transport header
+ */
+#define RTE_THASH_MAX_L4_LEN	\
+	((sizeof(union rte_thash_tuple)) / sizeof(uint32_t))
+
 /**
  * Prepare special converted key to use with rte_softrss_be()
  * @param orig
-- 
2.35.3

RE: [RFC 5/6] hash/thash: remove VLA warnings

[Morten Brørup]

> @@ -771,7 +771,7 @@ rte_thash_adjust_tuple(struct rte_thash_ctx *ctx,
>  	uint32_t desired_value,	unsigned int attempts,
>  	rte_thash_check_tuple_t fn, void *userdata)
>  {
> -	uint32_t tmp_tuple[tuple_len / sizeof(uint32_t)];
> +	uint32_t tmp_tuple[RTE_THASH_MAX_L4_LEN];

Keep the "/ sizeof(uint32_t)" here, rather than folding it into RTE_THASH_MAX_L4_LEN.

At your preference, keep RTE_THASH_MAX_L4_LEN (removing "in dwords" from it comment) or simply use:
uint32_t tmp_tuple[sizeof(union rte_thash_tuple) / sizeof(uint32_t)];

>  	unsigned int i, j, ret = 0;
>  	uint32_t hash, adj_bits;
>  	const uint8_t *hash_key;
> diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
> index 30b657e67a..322fe3af66 100644
> --- a/lib/hash/rte_thash.h
> +++ b/lib/hash/rte_thash.h
> @@ -108,6 +108,14 @@ union rte_thash_tuple {
>  	struct rte_ipv6_tuple	v6;
>  };
> 
> +/**
> + * maximum length in dwords of input tuple to
> + * calculate hash of ipv(4|6) header +
> + * transport header
> + */
> +#define RTE_THASH_MAX_L4_LEN	\
> +	((sizeof(union rte_thash_tuple)) / sizeof(uint32_t))
> +
>  /**
>   * Prepare special converted key to use with rte_softrss_be()
>   * @param orig
> --
> 2.35.3

With sizeof(uint32_t) back in tmp_tuple...
Acked-by: Morten Brørup <mb@smartsharesystems.com>

RE: [RFC 5/6] hash/thash: remove VLA warnings

[Konstantin Ananyev]

> > @@ -771,7 +771,7 @@ rte_thash_adjust_tuple(struct rte_thash_ctx *ctx,
> >  	uint32_t desired_value,	unsigned int attempts,
> >  	rte_thash_check_tuple_t fn, void *userdata)
> >  {
> > -	uint32_t tmp_tuple[tuple_len / sizeof(uint32_t)];
> > +	uint32_t tmp_tuple[RTE_THASH_MAX_L4_LEN];
> 
> Keep the "/ sizeof(uint32_t)" here, rather than folding it into RTE_THASH_MAX_L4_LEN.
> 
> At your preference, keep RTE_THASH_MAX_L4_LEN (removing "in dwords" from it comment) or simply use:
> uint32_t tmp_tuple[sizeof(union rte_thash_tuple) / sizeof(uint32_t)];

Not sure I got you here...
You are not happy with word "dwords" or ...?
Yes, it is size in 4B elems...
Same as   other RTE_THASH_*_LEN macros in the same .h 
 
> 
> >  	unsigned int i, j, ret = 0;
> >  	uint32_t hash, adj_bits;
> >  	const uint8_t *hash_key;
> > diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
> > index 30b657e67a..322fe3af66 100644
> > --- a/lib/hash/rte_thash.h
> > +++ b/lib/hash/rte_thash.h
> > @@ -108,6 +108,14 @@ union rte_thash_tuple {
> >  	struct rte_ipv6_tuple	v6;
> >  };
> >
> > +/**
> > + * maximum length in dwords of input tuple to
> > + * calculate hash of ipv(4|6) header +
> > + * transport header
> > + */
> > +#define RTE_THASH_MAX_L4_LEN	\
> > +	((sizeof(union rte_thash_tuple)) / sizeof(uint32_t))
> > +
> >  /**
> >   * Prepare special converted key to use with rte_softrss_be()
> >   * @param orig
> > --
> > 2.35.3
> 
> With sizeof(uint32_t) back in tmp_tuple...
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

RE: [RFC 5/6] hash/thash: remove VLA warnings

[Morten Brørup]

> > > @@ -771,7 +771,7 @@ rte_thash_adjust_tuple(struct rte_thash_ctx *ctx,
> > >  	uint32_t desired_value,	unsigned int attempts,
> > >  	rte_thash_check_tuple_t fn, void *userdata)
> > >  {
> > > -	uint32_t tmp_tuple[tuple_len / sizeof(uint32_t)];
> > > +	uint32_t tmp_tuple[RTE_THASH_MAX_L4_LEN];
> >
> > Keep the "/ sizeof(uint32_t)" here, rather than folding it into
> RTE_THASH_MAX_L4_LEN.
> >
> > At your preference, keep RTE_THASH_MAX_L4_LEN (removing "in dwords" from it
> comment) or simply use:
> > uint32_t tmp_tuple[sizeof(union rte_thash_tuple) / sizeof(uint32_t)];
> 
> Not sure I got you here...
> You are not happy with word "dwords" or ...?
> Yes, it is size in 4B elems...
> Same as   other RTE_THASH_*_LEN macros in the same .h

Sorry, I missed those. I only looked at the .c file and the description of the tuple_len parameter in the .h file.

Then the RFC is Acked as is. :-)

> 
> >
> > >  	unsigned int i, j, ret = 0;
> > >  	uint32_t hash, adj_bits;
> > >  	const uint8_t *hash_key;
> > > diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
> > > index 30b657e67a..322fe3af66 100644
> > > --- a/lib/hash/rte_thash.h
> > > +++ b/lib/hash/rte_thash.h
> > > @@ -108,6 +108,14 @@ union rte_thash_tuple {
> > >  	struct rte_ipv6_tuple	v6;
> > >  };
> > >
> > > +/**
> > > + * maximum length in dwords of input tuple to
> > > + * calculate hash of ipv(4|6) header +
> > > + * transport header
> > > + */
> > > +#define RTE_THASH_MAX_L4_LEN	\
> > > +	((sizeof(union rte_thash_tuple)) / sizeof(uint32_t))
> > > +
> > >  /**
> > >   * Prepare special converted key to use with rte_softrss_be()
> > >   * @param orig
> > > --
> > > 2.35.3
> >
> > With sizeof(uint32_t) back in tmp_tuple...
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>

[RFC 6/6] rcu: remove VLA warnings

[Konstantin Ananyev]

From: Konstantin Ananyev <konstantin.ananyev@huawei.com>

1) ./lib/rcu/rte_rcu_qsbr.c:359:9: warning: ISO C90 forbids variable length array ‘data’ [-Wvla]
2) ./lib/rcu/rte_rcu_qsbr.c:422:9: warning: ISO C90 forbids variable length array ‘data’ [-Wvla]

In both cases we allocate VLA for one element from RCU deferred queue.
Right now, size of element in RCU queue is not limited by API.
The approach is to introduce some reasonable limitation on RCU DQ
element size.
Choose 128B for now.
With that in place we can replace both VLA occurencies with fixed size
array.

Note that such change need to be treated as API change.
So can be applied only at 24.11.

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
 lib/rcu/rte_rcu_qsbr.c | 7 ++++---
 lib/rcu/rte_rcu_qsbr.h | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index f08d974d07..6800ef03a5 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -278,7 +278,8 @@ rte_rcu_qsbr_dq_create(const struct rte_rcu_qsbr_dq_parameters *params)
 	if (params == NULL || params->free_fn == NULL ||
 		params->v == NULL || params->name == NULL ||
 		params->size == 0 || params->esize == 0 ||
-		(params->esize % 4 != 0)) {
+		(params->esize % 4 != 0) ||
+		params->esize > RTE_QSBR_ESIZE_MAX) {
 		RCU_LOG(ERR, "Invalid input parameter");
 		rte_errno = EINVAL;
 
@@ -356,7 +357,7 @@ int rte_rcu_qsbr_dq_enqueue(struct rte_rcu_qsbr_dq *dq, void *e)
 		return 1;
 	}
 
-	char data[dq->esize];
+	char data[RTE_QSBR_ESIZE_MAX + __RTE_QSBR_TOKEN_SIZE];
 	dq_elem = (__rte_rcu_qsbr_dq_elem_t *)data;
 	/* Start the grace period */
 	dq_elem->token = rte_rcu_qsbr_start(dq->v);
@@ -419,7 +420,7 @@ rte_rcu_qsbr_dq_reclaim(struct rte_rcu_qsbr_dq *dq, unsigned int n,
 
 	cnt = 0;
 
-	char data[dq->esize];
+	char data[RTE_QSBR_ESIZE_MAX + __RTE_QSBR_TOKEN_SIZE];
 	/* Check reader threads quiescent state and reclaim resources */
 	while (cnt < n &&
 		rte_ring_dequeue_bulk_elem_start(dq->r, &data,
diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h
index 0506191b80..892e5a31a4 100644
--- a/lib/rcu/rte_rcu_qsbr.h
+++ b/lib/rcu/rte_rcu_qsbr.h
@@ -86,6 +86,11 @@ struct __rte_cache_aligned rte_rcu_qsbr_cnt {
 #define __RTE_QSBR_CNT_MAX ((uint64_t)~0)
 #define __RTE_QSBR_TOKEN_SIZE sizeof(uint64_t)
 
+/**
+ * Max allowable size (in bytes) of each element in the defer queue
+ */
+#define RTE_QSBR_ESIZE_MAX	(2 * RTE_CACHE_LINE_MIN_SIZE)
+
 /* RTE Quiescent State variable structure.
  * This structure has two elements that vary in size based on the
  * 'max_threads' parameter.
-- 
2.35.3

RE: [RFC 6/6] rcu: remove VLA warnings

[Morten Brørup]

> --- a/lib/rcu/rte_rcu_qsbr.h
> +++ b/lib/rcu/rte_rcu_qsbr.h
> @@ -86,6 +86,11 @@ struct __rte_cache_aligned rte_rcu_qsbr_cnt {
>  #define __RTE_QSBR_CNT_MAX ((uint64_t)~0)
>  #define __RTE_QSBR_TOKEN_SIZE sizeof(uint64_t)
> 
> +/**
> + * Max allowable size (in bytes) of each element in the defer queue
> + */
> +#define RTE_QSBR_ESIZE_MAX	(2 * RTE_CACHE_LINE_MIN_SIZE)

Consider moving this to /config/rte_config.h

With or without suggested change...
Acked-By: Morten Brørup <mb@smartsharesystems.com>