[v6,01/14] examples/l3fwd: fix queue ID restriction
Checks
Commit Message
Currently application supports queue IDs up to 255
and max queues of 256 irrespective of device support.
This limits the number of active lcores to 256.
The patch fixes these constraints by increasing
the queue IDs to support up to 65535.
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
examples/l3fwd/l3fwd.h | 2 +-
examples/l3fwd/l3fwd_acl.c | 4 ++--
examples/l3fwd/l3fwd_em.c | 4 ++--
examples/l3fwd/l3fwd_event.h | 2 +-
examples/l3fwd/l3fwd_fib.c | 4 ++--
examples/l3fwd/l3fwd_lpm.c | 5 ++---
examples/l3fwd/main.c | 28 ++++++++++++++++------------
7 files changed, 26 insertions(+), 23 deletions(-)
Comments
Hello,
On Thu, Mar 21, 2024 at 7:48 PM Sivaprasad Tummala
<sivaprasad.tummala@amd.com> wrote:
>
> Currently application supports queue IDs up to 255
I think it only relates to Rx queue IDs.
Before this patch, the Tx queue count is already stored as a uint32_t
or uint16_t and checked against RTE_MAX_LCORE.
So no limit on the Tx queue count side.
Can you just adjust the commitlog accordingly?
(One may argue that the Tx queue count should be also checked against
RTE_MAX_QUEUES_PER_PORT, but it is a separate issue to this patch and
in practice, we probably always have RTE_MAX_QUEUES_PER_PORT >
RTE_MAX_LCORE).
> and max queues of 256 irrespective of device support.
> This limits the number of active lcores to 256.
>
> The patch fixes these constraints by increasing
> the queue IDs to support up to 65535.
[snip]
> diff --git a/examples/l3fwd/l3fwd_acl.c b/examples/l3fwd/l3fwd_acl.c
> index 401692bcec..2bd63181bc 100644
> --- a/examples/l3fwd/l3fwd_acl.c
> +++ b/examples/l3fwd/l3fwd_acl.c
> @@ -997,7 +997,7 @@ acl_main_loop(__rte_unused void *dummy)
> uint64_t prev_tsc, diff_tsc, cur_tsc;
> int i, nb_rx;
> uint16_t portid;
> - uint8_t queueid;
> + uint16_t queueid;
> struct lcore_conf *qconf;
> int socketid;
> const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1)
> @@ -1020,7 +1020,7 @@ acl_main_loop(__rte_unused void *dummy)
> portid = qconf->rx_queue_list[i].port_id;
> queueid = qconf->rx_queue_list[i].queue_id;
> RTE_LOG(INFO, L3FWD,
> - " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
> + " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
Nit: should be %PRIu16 (idem in other hunks formatting a queue).
> lcore_id, portid, queueid);
> }
>
[snip]
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 8d32ae1dd5..4d4738b92b 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
[snip]
> @@ -366,7 +366,7 @@ init_lcore_rx_queues(void)
> nb_rx_queue = lcore_conf[lcore].n_rx_queue;
> if (nb_rx_queue >= MAX_RX_QUEUE_PER_LCORE) {
> printf("error: too many queues (%u) for lcore: %u\n",
> - (unsigned)nb_rx_queue + 1, (unsigned)lcore);
> + (unsigned int)nb_rx_queue + 1, (unsigned int)lcore);
Nit: this does not seem related to the patch (probably a split issue,
as a later patch touches this part of the code too).
> return -1;
> } else {
> lcore_conf[lcore].rx_queue_list[nb_rx_queue].port_id =
> @@ -500,6 +500,8 @@ parse_config(const char *q_arg)
> char *str_fld[_NUM_FLD];
> int i;
> unsigned size;
> + uint16_t max_fld[_NUM_FLD] = {USHRT_MAX,
> + USHRT_MAX, UCHAR_MAX};
Nit: no newline.
This part validates user input for the rx queue used by a lcore.
Some later check in the example (or in ethdev) may raise an error if
requesting too many queues, but I think the limit here should be
RTE_MAX_QUEUES_PER_PORT.
Besides, this hunk also changes the check on max port and max lcore.
This is something that should be left untouched at this point of the series.
I would expect something like:
uint16_t max_fld[_NUM_FLD] = {255, RTE_MAX_QUEUES_PER_PORT, 255};
>
> nb_lcore_params = 0;
>
> @@ -518,7 +520,8 @@ parse_config(const char *q_arg)
> for (i = 0; i < _NUM_FLD; i++){
> errno = 0;
> int_fld[i] = strtoul(str_fld[i], &end, 0);
> - if (errno != 0 || end == str_fld[i] || int_fld[i] > 255)
> + if (errno != 0 || end == str_fld[i] || int_fld[i] >
> + max_fld[i])
Nit: no newline.
> return -1;
> }
> if (nb_lcore_params >= MAX_LCORE_PARAMS) {
[snip]
The other changes on the l3fwd example code in this series look good to me.
Thanks.
[AMD Official Use Only - General]
Hi,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, March 22, 2024 9:11 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
> Cc: david.hunt@intel.com; anatoly.burakov@intel.com; jerinj@marvell.com;
> radu.nicolau@intel.com; gakhil@marvell.com; cristian.dumitrescu@intel.com; Yigit,
> Ferruh <Ferruh.Yigit@amd.com>; konstantin.ananyev@huawei.com;
> stephen@networkplumber.org; mb@smartsharesystems.com;
> thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v6 01/14] examples/l3fwd: fix queue ID restriction
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hello,
>
> On Thu, Mar 21, 2024 at 7:48 PM Sivaprasad Tummala
> <sivaprasad.tummala@amd.com> wrote:
> >
> > Currently application supports queue IDs up to 255
>
> I think it only relates to Rx queue IDs.
>
> Before this patch, the Tx queue count is already stored as a uint32_t or uint16_t
> and checked against RTE_MAX_LCORE.
> So no limit on the Tx queue count side.
>
> Can you just adjust the commitlog accordingly?
OK
>
>
> (One may argue that the Tx queue count should be also checked against
> RTE_MAX_QUEUES_PER_PORT, but it is a separate issue to this patch and in
> practice, we probably always have RTE_MAX_QUEUES_PER_PORT >
> RTE_MAX_LCORE).
>
>
> > and max queues of 256 irrespective of device support.
> > This limits the number of active lcores to 256.
> >
> > The patch fixes these constraints by increasing the queue IDs to
> > support up to 65535.
>
> [snip]
>
> > diff --git a/examples/l3fwd/l3fwd_acl.c b/examples/l3fwd/l3fwd_acl.c
> > index 401692bcec..2bd63181bc 100644
> > --- a/examples/l3fwd/l3fwd_acl.c
> > +++ b/examples/l3fwd/l3fwd_acl.c
> > @@ -997,7 +997,7 @@ acl_main_loop(__rte_unused void *dummy)
> > uint64_t prev_tsc, diff_tsc, cur_tsc;
> > int i, nb_rx;
> > uint16_t portid;
> > - uint8_t queueid;
> > + uint16_t queueid;
> > struct lcore_conf *qconf;
> > int socketid;
> > const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1)
> > @@ -1020,7 +1020,7 @@ acl_main_loop(__rte_unused void *dummy)
> > portid = qconf->rx_queue_list[i].port_id;
> > queueid = qconf->rx_queue_list[i].queue_id;
> > RTE_LOG(INFO, L3FWD,
> > - " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
> > + " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
>
> Nit: should be %PRIu16 (idem in other hunks formatting a queue).
OK
>
>
> > lcore_id, portid, queueid);
> > }
> >
>
> [snip]
>
>
> > diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c index
> > 8d32ae1dd5..4d4738b92b 100644
> > --- a/examples/l3fwd/main.c
> > +++ b/examples/l3fwd/main.c
>
> [snip]
>
>
> > @@ -366,7 +366,7 @@ init_lcore_rx_queues(void)
> > nb_rx_queue = lcore_conf[lcore].n_rx_queue;
> > if (nb_rx_queue >= MAX_RX_QUEUE_PER_LCORE) {
> > printf("error: too many queues (%u) for lcore: %u\n",
> > - (unsigned)nb_rx_queue + 1, (unsigned)lcore);
> > + (unsigned int)nb_rx_queue + 1,
> > + (unsigned int)lcore);
>
> Nit: this does not seem related to the patch (probably a split issue, as a later patch
> touches this part of the code too).
Yes, this was done to avoid checkpatch error.
>
>
> > return -1;
> > } else {
> >
> > lcore_conf[lcore].rx_queue_list[nb_rx_queue].port_id = @@ -500,6 +500,8 @@
> parse_config(const char *q_arg)
> > char *str_fld[_NUM_FLD];
> > int i;
> > unsigned size;
> > + uint16_t max_fld[_NUM_FLD] = {USHRT_MAX,
> > + USHRT_MAX, UCHAR_MAX};
>
> Nit: no newline.
>
> This part validates user input for the rx queue used by a lcore.
> Some later check in the example (or in ethdev) may raise an error if requesting too
> many queues, but I think the limit here should be RTE_MAX_QUEUES_PER_PORT.
>
> Besides, this hunk also changes the check on max port and max lcore.
> This is something that should be left untouched at this point of the series.
>
> I would expect something like:
> uint16_t max_fld[_NUM_FLD] = {255, RTE_MAX_QUEUES_PER_PORT, 255};
I agree on the RTE_MAX_QUEUES_PER_PORT, but port_id is already uint16_t and
Hence USHRT_MAX is relevant and similarly UCHAR_MAX expands to 255.
>
>
> >
> > nb_lcore_params = 0;
> >
> > @@ -518,7 +520,8 @@ parse_config(const char *q_arg)
> > for (i = 0; i < _NUM_FLD; i++){
> > errno = 0;
> > int_fld[i] = strtoul(str_fld[i], &end, 0);
> > - if (errno != 0 || end == str_fld[i] || int_fld[i] > 255)
> > + if (errno != 0 || end == str_fld[i] || int_fld[i] >
> > +
> > + max_fld[i])
>
> Nit: no newline.
OK
>
> > return -1;
> > }
> > if (nb_lcore_params >= MAX_LCORE_PARAMS) {
>
> [snip]
>
>
> The other changes on the l3fwd example code in this series look good to me.
>
>
> Thanks.
>
> --
> David Marchand
@@ -74,7 +74,7 @@ struct mbuf_table {
struct lcore_rx_queue {
uint16_t port_id;
- uint8_t queue_id;
+ uint16_t queue_id;
} __rte_cache_aligned;
struct lcore_conf {
@@ -997,7 +997,7 @@ acl_main_loop(__rte_unused void *dummy)
uint64_t prev_tsc, diff_tsc, cur_tsc;
int i, nb_rx;
uint16_t portid;
- uint8_t queueid;
+ uint16_t queueid;
struct lcore_conf *qconf;
int socketid;
const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1)
@@ -1020,7 +1020,7 @@ acl_main_loop(__rte_unused void *dummy)
portid = qconf->rx_queue_list[i].port_id;
queueid = qconf->rx_queue_list[i].queue_id;
RTE_LOG(INFO, L3FWD,
- " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+ " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
lcore_id, portid, queueid);
}
@@ -586,7 +586,7 @@ em_main_loop(__rte_unused void *dummy)
unsigned lcore_id;
uint64_t prev_tsc, diff_tsc, cur_tsc;
int i, nb_rx;
- uint8_t queueid;
+ uint16_t queueid;
uint16_t portid;
struct lcore_conf *qconf;
const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
@@ -609,7 +609,7 @@ em_main_loop(__rte_unused void *dummy)
portid = qconf->rx_queue_list[i].port_id;
queueid = qconf->rx_queue_list[i].queue_id;
RTE_LOG(INFO, L3FWD,
- " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+ " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
lcore_id, portid, queueid);
}
@@ -78,8 +78,8 @@ struct l3fwd_event_resources {
uint8_t deq_depth;
uint8_t has_burst;
uint8_t enabled;
- uint8_t eth_rx_queues;
uint8_t vector_enabled;
+ uint16_t eth_rx_queues;
uint16_t vector_size;
uint64_t vector_tmo_ns;
};
@@ -186,7 +186,7 @@ fib_main_loop(__rte_unused void *dummy)
uint64_t prev_tsc, diff_tsc, cur_tsc;
int i, nb_rx;
uint16_t portid;
- uint8_t queueid;
+ uint16_t queueid;
struct lcore_conf *qconf;
const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
US_PER_S * BURST_TX_DRAIN_US;
@@ -208,7 +208,7 @@ fib_main_loop(__rte_unused void *dummy)
portid = qconf->rx_queue_list[i].port_id;
queueid = qconf->rx_queue_list[i].queue_id;
RTE_LOG(INFO, L3FWD,
- " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+ " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
lcore_id, portid, queueid);
}
@@ -148,8 +148,7 @@ lpm_main_loop(__rte_unused void *dummy)
unsigned lcore_id;
uint64_t prev_tsc, diff_tsc, cur_tsc;
int i, nb_rx;
- uint16_t portid;
- uint8_t queueid;
+ uint16_t portid, queueid;
struct lcore_conf *qconf;
const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1) /
US_PER_S * BURST_TX_DRAIN_US;
@@ -171,7 +170,7 @@ lpm_main_loop(__rte_unused void *dummy)
portid = qconf->rx_queue_list[i].port_id;
queueid = qconf->rx_queue_list[i].queue_id;
RTE_LOG(INFO, L3FWD,
- " -- lcoreid=%u portid=%u rxqueueid=%hhu\n",
+ " -- lcoreid=%u portid=%u rxqueueid=%hu\n",
lcore_id, portid, queueid);
}
@@ -98,7 +98,7 @@ struct parm_cfg parm_config;
struct lcore_params {
uint16_t port_id;
- uint8_t queue_id;
+ uint16_t queue_id;
uint8_t lcore_id;
} __rte_cache_aligned;
@@ -292,14 +292,14 @@ setup_l3fwd_lookup_tables(void)
static int
check_lcore_params(void)
{
- uint8_t queue, lcore;
- uint16_t i;
+ uint16_t queue, i;
+ uint8_t lcore;
int socketid;
for (i = 0; i < nb_lcore_params; ++i) {
queue = lcore_params[i].queue_id;
if (queue >= MAX_RX_QUEUE_PER_PORT) {
- printf("invalid queue number: %hhu\n", queue);
+ printf("invalid queue number: %hu\n", queue);
return -1;
}
lcore = lcore_params[i].lcore_id;
@@ -336,7 +336,7 @@ check_port_config(void)
return 0;
}
-static uint8_t
+static uint16_t
get_port_n_rx_queues(const uint16_t port)
{
int queue = -1;
@@ -352,7 +352,7 @@ get_port_n_rx_queues(const uint16_t port)
lcore_params[i].port_id);
}
}
- return (uint8_t)(++queue);
+ return (uint16_t)(++queue);
}
static int
@@ -366,7 +366,7 @@ init_lcore_rx_queues(void)
nb_rx_queue = lcore_conf[lcore].n_rx_queue;
if (nb_rx_queue >= MAX_RX_QUEUE_PER_LCORE) {
printf("error: too many queues (%u) for lcore: %u\n",
- (unsigned)nb_rx_queue + 1, (unsigned)lcore);
+ (unsigned int)nb_rx_queue + 1, (unsigned int)lcore);
return -1;
} else {
lcore_conf[lcore].rx_queue_list[nb_rx_queue].port_id =
@@ -500,6 +500,8 @@ parse_config(const char *q_arg)
char *str_fld[_NUM_FLD];
int i;
unsigned size;
+ uint16_t max_fld[_NUM_FLD] = {USHRT_MAX,
+ USHRT_MAX, UCHAR_MAX};
nb_lcore_params = 0;
@@ -518,7 +520,8 @@ parse_config(const char *q_arg)
for (i = 0; i < _NUM_FLD; i++){
errno = 0;
int_fld[i] = strtoul(str_fld[i], &end, 0);
- if (errno != 0 || end == str_fld[i] || int_fld[i] > 255)
+ if (errno != 0 || end == str_fld[i] || int_fld[i] >
+ max_fld[i])
return -1;
}
if (nb_lcore_params >= MAX_LCORE_PARAMS) {
@@ -529,7 +532,7 @@ parse_config(const char *q_arg)
lcore_params_array[nb_lcore_params].port_id =
(uint8_t)int_fld[FLD_PORT];
lcore_params_array[nb_lcore_params].queue_id =
- (uint8_t)int_fld[FLD_QUEUE];
+ (uint16_t)int_fld[FLD_QUEUE];
lcore_params_array[nb_lcore_params].lcore_id =
(uint8_t)int_fld[FLD_LCORE];
++nb_lcore_params;
@@ -630,7 +633,7 @@ parse_event_eth_rx_queues(const char *eth_rx_queues)
{
struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc();
char *end = NULL;
- uint8_t num_eth_rx_queues;
+ uint16_t num_eth_rx_queues;
/* parse decimal string */
num_eth_rx_queues = strtoul(eth_rx_queues, &end, 10);
@@ -1211,7 +1214,8 @@ config_port_max_pkt_len(struct rte_eth_conf *conf,
static void
l3fwd_poll_resource_setup(void)
{
- uint8_t nb_rx_queue, queue, socketid;
+ uint8_t socketid;
+ uint16_t nb_rx_queue, queue;
struct rte_eth_dev_info dev_info;
uint32_t n_tx_queue, nb_lcores;
struct rte_eth_txconf *txconf;
@@ -1535,7 +1539,7 @@ main(int argc, char **argv)
struct lcore_conf *qconf;
uint16_t queueid, portid;
unsigned int lcore_id;
- uint8_t queue;
+ uint16_t queue;
int ret;
/* init EAL */