[PATCH 1/6] examples/l3fwd: fix lcore ID restriction

[Morten Brørup]

> From: David Marchand [mailto:david.marchand at redhat.com]
> Sent: Thursday, 7 March 2024 09.34
> 
> On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> <sivaprasad.tummala at 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: stable at dpdk.org
> 
> Please remove this request for backport in the next revision.

Disagree, see comment above.

> 
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala at 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).