[1/6] examples/l3fwd: fix lcore ID restriction
Checks
Commit Message
Currently the config option allows lcore IDs up to 255,
irrespective of RTE_MAX_LCORES and needs to be fixed.
The patch allows config options based on DPDK config.
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
examples/l3fwd/main.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Comments
With modern CPUs, it is possible to have higher
CPU count thus we can have higher RTE_MAX_LCORES.
In DPDK sample applications, the current config
lcore options are hard limited to 255.
The patchset fixes these constraints by allowing
all lcore IDs up to RTE_MAX_LCORES.
Sivaprasad Tummala (6):
examples/l3fwd: fix lcore ID restriction
examples/l3fwd-power: fix lcore ID restriction
examples/l3fwd-graph: fix lcore ID restriction
examples/ipsec-secgw: fix lcore ID restriction
examples/qos_sched: fix lcore ID restriction
examples/vm_power_manager: fix lcore ID restriction
examples/ipsec-secgw/event_helper.h | 2 +-
examples/ipsec-secgw/ipsec-secgw.c | 16 +++++++++-------
examples/ipsec-secgw/ipsec.c | 2 +-
examples/l3fwd-graph/main.c | 14 +++++++-------
examples/l3fwd-power/main.c | 16 +++++++++-------
examples/l3fwd-power/main.h | 2 +-
examples/l3fwd/main.c | 19 +++++++++++--------
examples/qos_sched/args.c | 6 +++---
.../guest_cli/vm_power_cli_guest.c | 4 ++--
9 files changed, 44 insertions(+), 37 deletions(-)
On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
<sivaprasad.tummala@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.
>
> The patch allows config options based on DPDK config.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
Please remove this request for backport in the next revision.
>
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@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.
> } __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.
> + 255, RTE_MAX_LCORE};
>
> 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) {
> @@ -531,7 +534,7 @@ parse_config(const char *q_arg)
> lcore_params_array[nb_lcore_params].queue_id =
> (uint8_t)int_fld[FLD_QUEUE];
> lcore_params_array[nb_lcore_params].lcore_id =
> - (uint8_t)int_fld[FLD_LCORE];
> + (uint16_t)int_fld[FLD_LCORE];
> ++nb_lcore_params;
> }
> lcore_params = lcore_params_array;
> --
> 2.25.1
>
I did not check other patches in the series, but I suggest you revisit
them in the light of those comments.
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 7 March 2024 09.34
>
> On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> <sivaprasad.tummala@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@dpdk.org
>
> Please remove this request for backport in the next revision.
Disagree, see comment above.
>
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@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).
On Thu, Mar 7, 2024 at 10:16 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 7 March 2024 09.34
> >
> > On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> > <sivaprasad.tummala@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.
This is not a bug.
And with careful configuration (using --lcores), you can already start
l3fwd with 254 datapath threads, right?
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 7 March 2024 10.22
restriction
>
> On Thu, Mar 7, 2024 at 10:16 AM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Thursday, 7 March 2024 09.34
> > >
> > > On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> > > <sivaprasad.tummala@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.
>
> This is not a bug.
DPDK 23.11 LTS supposedly supports Zen 4, which has 512 cores [1].
If l3fwd does not support it, it is a bug in l3fwd.
[1]: https://elixir.bootlin.com/dpdk/v23.11/source/config/x86/meson.build#L88
>
> And with careful configuration (using --lcores), you can already start
> l3fwd with 254 datapath threads, right?
Correct, but you cannot start l3fwd with all 512 cores.
There should be some test scenario to set up l3fwd to use all 512 cores.
If there are practical limitations that effectively prevents l3fwd from using all 512 cores, e.g. if no NIC offers more than 256 queues, then I'll let go and not consider it a bug.
>
>
> --
> David Marchand
[AMD Official Use Only - General]
Hi,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, March 7, 2024 2:04 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;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH 1/6] examples/l3fwd: fix lcore ID restriction
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Mon, Dec 18, 2023 at 8:49 AM Sivaprasad Tummala
> <sivaprasad.tummala@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.
>
> >
> > The patch allows config options based on DPDK config.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
>
> Please remove this request for backport in the next revision.
>
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@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.
Will fix this to be aligned to EAL. However, I don't think of a SOC/CPU with more than
65536 cores/threads.
>
>
> > } __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.
OK! Will fix it
>
> 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.
queue_id is already modified as uint16_t based on the earlier comment from Konstantin.
The port_id is already uint16_t even if RTE_MAX_ETHPORTS exceeds 255.
I will fix the max_fld type to "uint32_t" to accommodate lcore.
>
>
> > + 255, RTE_MAX_LCORE};
> >
> > 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) { @@ -531,7
> > +534,7 @@ parse_config(const char *q_arg)
> > lcore_params_array[nb_lcore_params].queue_id =
> > (uint8_t)int_fld[FLD_QUEUE];
> > lcore_params_array[nb_lcore_params].lcore_id =
> > - (uint8_t)int_fld[FLD_LCORE];
> > + (uint16_t)int_fld[FLD_LCORE];
> > ++nb_lcore_params;
> > }
> > lcore_params = lcore_params_array;
> > --
> > 2.25.1
> >
>
> I did not check other patches in the series, but I suggest you revisit them in the
> light of those comments.
OK
>
>
> --
> David Marchand
Thanks & Regards,
Sivaprasad
@@ -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;
} __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,
+ 255, RTE_MAX_LCORE};
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) {
@@ -531,7 +534,7 @@ parse_config(const char *q_arg)
lcore_params_array[nb_lcore_params].queue_id =
(uint8_t)int_fld[FLD_QUEUE];
lcore_params_array[nb_lcore_params].lcore_id =
- (uint8_t)int_fld[FLD_LCORE];
+ (uint16_t)int_fld[FLD_LCORE];
++nb_lcore_params;
}
lcore_params = lcore_params_array;