> From: David Marchand [mailto:david.march...@redhat.com]
> Sent: Thursday, 7 March 2024 09.34
>
> On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> <sivaprasad.tumm...@amd.com> wrote:
> >
> > Currently the config option allows lcore IDs up to 255,
> > irrespective of RTE_MAX_LCORES and needs to be fixed.
>
> "needs to be fixed" ?
> I disagree on the principle.
> The examples were written with limitations, this is not a bug.
Unfortunately, l3fwd is not only an example; it is also used for benchmarking.
It really belongs in some other directory.
With that in mind, I would consider it a bug that the benchmarking application
cannot handle the amount of cores available in modern CPUs.
>
> >
> > The patch allows config options based on DPDK config.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: sta...@dpdk.org
>
> Please remove this request for backport in the next revision.
Disagree, see comment above.
>
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tumm...@amd.com>
> > ---
> > examples/l3fwd/main.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> > index 3bf28aec0c..847ded0ad2 100644
> > --- a/examples/l3fwd/main.c
> > +++ b/examples/l3fwd/main.c
> > @@ -99,7 +99,7 @@ struct parm_cfg parm_config;
> > struct lcore_params {
> > uint16_t port_id;
> > uint8_t queue_id;
> > - uint8_t lcore_id;
> > + uint16_t lcore_id;
>
> lcore_id are stored as an unsigned int (so potentially 32bits) in EAL.
> Moving to uint16_t seems not enough.
<rant>
I might say that the lcore_id API type was not well thought through when it was
decided to use unsigned int.
The port_id and queue_id APIs have been updated to use uint16_t.
If the application uses one TX queue per lcore, using the same type for
lcore_id as for queue_id should suffice, i.e. uint16_t.
It's unlikely that the lcore_id API type is going to change; we are stuck with
unsigned int, although uint16_t would probably be better (to prevent type
conversion related bugs).
</rant>
That said, you can follow David's advice and use unsigned int for lcore_id, or
you can use uint16_t and add a build time check after the structure:
static_assert(RTE_MAX_LCORE <= UINT16_MAX + 1, "lcore_id does not fit into
uint16_t");
>
>
> > } __rte_cache_aligned;
> >
> > static struct lcore_params lcore_params_array[MAX_LCORE_PARAMS];
> > @@ -292,8 +292,8 @@ setup_l3fwd_lookup_tables(void)
> > static int
> > check_lcore_params(void)
> > {
> > - uint8_t queue, lcore;
> > - uint16_t i;
> > + uint8_t queue;
> > + uint16_t i, lcore;
> > int socketid;
> >
> > for (i = 0; i < nb_lcore_params; ++i) {
> > @@ -359,7 +359,7 @@ static int
> > init_lcore_rx_queues(void)
> > {
> > uint16_t i, nb_rx_queue;
> > - uint8_t lcore;
> > + uint16_t lcore;
> >
> > for (i = 0; i < nb_lcore_params; ++i) {
> > lcore = lcore_params[i].lcore_id;
> > @@ -500,6 +500,8 @@ parse_config(const char *q_arg)
> > char *str_fld[_NUM_FLD];
> > int i;
> > unsigned size;
> > + unsigned int max_fld[_NUM_FLD] = {RTE_MAX_ETHPORTS,
>
> This change here is not described in the commitlog and introduces a bug.
>
> In this example, queue_id is stored as a uint8_t.
> queue_id are stored as uint16_t in ethdev.
> Yet RTE_MAX_ETHPORTS can be larger than 255.
The API type of port_id is uint16_t, so RTE_MAX_ETHPORTS can be up to
UINT16_MAX (65535).
