* [RFC 1/6] eal/linux: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 12:23 ` Morten Brørup
2024-04-18 10:33 ` [RFC 2/6] eal/common: " Konstantin Ananyev
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 1/6] eal/linux: remove VLA warnings
2024-04-18 10:33 ` [RFC 1/6] eal/linux: " Konstantin Ananyev
@ 2024-04-19 12:23 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 12:23 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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>
> ---
Acked-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 2/6] eal/common: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
2024-04-18 10:33 ` [RFC 1/6] eal/linux: " Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 11:54 ` Morten Brørup
2024-04-18 10:33 ` [RFC 3/6] ethdev: " Konstantin Ananyev
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 2/6] eal/common: remove VLA warnings
2024-04-18 10:33 ` [RFC 2/6] eal/common: " Konstantin Ananyev
@ 2024-04-19 11:54 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 11:54 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, Konstantin Ananyev
> 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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 3/6] ethdev: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
2024-04-18 10:33 ` [RFC 1/6] eal/linux: " Konstantin Ananyev
2024-04-18 10:33 ` [RFC 2/6] eal/common: " Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 12:06 ` Morten Brørup
2024-04-18 10:33 ` [RFC 4/6] hash: " Konstantin Ananyev
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 3/6] ethdev: remove VLA warnings
2024-04-18 10:33 ` [RFC 3/6] ethdev: " Konstantin Ananyev
@ 2024-04-19 12:06 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 12:06 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, Konstantin Ananyev
> 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!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 4/6] hash: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
` (2 preceding siblings ...)
2024-04-18 10:33 ` [RFC 3/6] ethdev: " Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 12:08 ` Morten Brørup
2024-04-18 10:33 ` [RFC 5/6] hash/thash: " Konstantin Ananyev
2024-04-18 10:33 ` [RFC 6/6] rcu: " Konstantin Ananyev
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 4/6] hash: remove VLA warnings
2024-04-18 10:33 ` [RFC 4/6] hash: " Konstantin Ananyev
@ 2024-04-19 12:08 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 12:08 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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>
> ---
Acked-by: Morten Brørup <mb@smartsharesystems.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 5/6] hash/thash: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
` (3 preceding siblings ...)
2024-04-18 10:33 ` [RFC 4/6] hash: " Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 12:17 ` Morten Brørup
2024-04-18 10:33 ` [RFC 6/6] rcu: " Konstantin Ananyev
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 5/6] hash/thash: remove VLA warnings
2024-04-18 10:33 ` [RFC 5/6] hash/thash: " Konstantin Ananyev
@ 2024-04-19 12:17 ` Morten Brørup
2024-04-19 12:53 ` Konstantin Ananyev
0 siblings, 1 reply; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 12:17 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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)];
> 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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 5/6] hash/thash: remove VLA warnings
2024-04-19 12:17 ` Morten Brørup
@ 2024-04-19 12:53 ` Konstantin Ananyev
2024-04-19 13:10 ` Morten Brørup
0 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-19 12:53 UTC (permalink / raw)
To: Morten Brørup, Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla
> > @@ -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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 5/6] hash/thash: remove VLA warnings
2024-04-19 12:53 ` Konstantin Ananyev
@ 2024-04-19 13:10 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 13:10 UTC (permalink / raw)
To: Konstantin Ananyev, Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla
> > > @@ -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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC 6/6] rcu: remove VLA warnings
2024-04-18 10:33 [RFC 0/6] core libs: remove VLA warnings Konstantin Ananyev
` (4 preceding siblings ...)
2024-04-18 10:33 ` [RFC 5/6] hash/thash: " Konstantin Ananyev
@ 2024-04-18 10:33 ` Konstantin Ananyev
2024-04-19 12:21 ` Morten Brørup
5 siblings, 1 reply; 15+ messages in thread
From: Konstantin Ananyev @ 2024-04-18 10:33 UTC (permalink / raw)
To: dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC 6/6] rcu: remove VLA warnings
2024-04-18 10:33 ` [RFC 6/6] rcu: " Konstantin Ananyev
@ 2024-04-19 12:21 ` Morten Brørup
0 siblings, 0 replies; 15+ messages in thread
From: Morten Brørup @ 2024-04-19 12:21 UTC (permalink / raw)
To: Konstantin Ananyev, dev
Cc: thomas, ferruh.yigit, andrew.rybchenko, yipeng1.wang,
sameh.gobriel, bruce.richardson, vladimir.medvedkin,
honnappa.nagarahalli, roretzla, Konstantin Ananyev
> --- 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>
^ permalink raw reply [flat|nested] 15+ messages in thread