[v5,1/4] examples/multi_process/client_server_mp: check port validity
Checks
Commit Message
From: Stephen Hemminger <sthemmin@microsoft.com>
The mp_server incorrectly allows a port mask that included hidden
ports and which later caused either lost packets or failed initialization.
This fixes explicitly checking that each bit in portmask is a
valid port before using it.
Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
.../client_server_mp/mp_server/args.c | 35 ++++++++++---------
.../client_server_mp/mp_server/args.h | 2 +-
.../client_server_mp/mp_server/init.c | 7 ++--
3 files changed, 22 insertions(+), 22 deletions(-)
Comments
Hi Stephen
One more small comment inline
From: Stephen Hemminger
> Sent: Friday, August 2, 2019 5:58 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
>
> From: Stephen Hemminger <sthemmin@microsoft.com>
>
> The mp_server incorrectly allows a port mask that included hidden ports and
> which later caused either lost packets or failed initialization.
>
> This fixes explicitly checking that each bit in portmask is a valid port before
> using it.
>
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
> .../client_server_mp/mp_server/args.c | 35 ++++++++++---------
> .../client_server_mp/mp_server/args.h | 2 +-
> .../client_server_mp/mp_server/init.c | 7 ++--
> 3 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> b/examples/multi_process/client_server_mp/mp_server/args.c
> index b0d8d7665c85..fdc008b3d677 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.c
> +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> @@ -10,6 +10,7 @@
> #include <errno.h>
>
> #include <rte_memory.h>
> +#include <rte_ethdev.h>
> #include <rte_string_fns.h>
>
> #include "common.h"
> @@ -41,31 +42,33 @@ usage(void)
> * array variable
> */
> static int
> -parse_portmask(uint8_t max_ports, const char *portmask)
> +parse_portmask(const char *portmask)
> {
> char *end = NULL;
> unsigned long pm;
> - uint16_t count = 0;
> + uint16_t id;
>
> if (portmask == NULL || *portmask == '\0')
> return -1;
>
> /* convert parameter to a number and verify */
> pm = strtoul(portmask, &end, 16);
> - if (end == NULL || *end != '\0' || pm == 0)
> + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
Why pm > UINT16_MAX ? should be something like > (1 << RTE_MAX_ETHPORTS) - 1.
And need to be sure pm type can hold RTE_MAX_ETHPORTS bits, otherwise port 0 may unlikely be all the time visible in the loop below.
> return -1;
>
> - /* loop through bits of the mask and mark ports */
> - while (pm != 0){
> - if (pm & 0x01){ /* bit is set in mask, use port */
> - if (count >= max_ports)
> - printf("WARNING: requested port %u not
> present"
> - " - ignoring\n", (unsigned)count);
> - else
> - ports->id[ports->num_ports++] = count;
> - }
> - pm = (pm >> 1);
> - count++;
> + RTE_ETH_FOREACH_DEV(id) {
> + unsigned long msk = 1u << id;
> +
> + if ((pm & msk) == 0)
> + continue;
> +
> + pm &= ~msk;
> + ports->id[ports->num_ports++] = id;
> + }
> +
> + if (pm != 0) {
> + printf("WARNING: leftover ports in mask %#lx - ignoring\n",
> + pm);
> }
>
> return 0;
> @@ -99,7 +102,7 @@ parse_num_clients(const char *clients)
> * on error.
> */
> int
> -parse_app_args(uint16_t max_ports, int argc, char *argv[])
> +parse_app_args(int argc, char *argv[])
> {
> int option_index, opt;
> char **argvopt = argv;
> @@ -112,7 +115,7 @@ parse_app_args(uint16_t max_ports, int argc, char
> *argv[])
> &option_index)) != EOF){
> switch (opt){
> case 'p':
> - if (parse_portmask(max_ports, optarg) != 0){
> + if (parse_portmask(optarg) != 0) {
> usage();
> return -1;
> }
> diff --git a/examples/multi_process/client_server_mp/mp_server/args.h
> b/examples/multi_process/client_server_mp/mp_server/args.h
> index 79c190a33a37..52c8cc86e6f0 100644
> --- a/examples/multi_process/client_server_mp/mp_server/args.h
> +++ b/examples/multi_process/client_server_mp/mp_server/args.h
> @@ -5,6 +5,6 @@
> #ifndef _ARGS_H_
> #define _ARGS_H_
>
> -int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
> +int parse_app_args(int argc, char *argv[]);
>
> #endif /* ifndef _ARGS_H_ */
> diff --git a/examples/multi_process/client_server_mp/mp_server/init.c
> b/examples/multi_process/client_server_mp/mp_server/init.c
> index 3af5dc6994bf..1b0569937b51 100644
> --- a/examples/multi_process/client_server_mp/mp_server/init.c
> +++ b/examples/multi_process/client_server_mp/mp_server/init.c
> @@ -238,7 +238,7 @@ init(int argc, char *argv[]) {
> int retval;
> const struct rte_memzone *mz;
> - uint16_t i, total_ports;
> + uint16_t i;
>
> /* init EAL, parsing EAL args */
> retval = rte_eal_init(argc, argv);
> @@ -247,9 +247,6 @@ init(int argc, char *argv[])
> argc -= retval;
> argv += retval;
>
> - /* get total number of ports */
> - total_ports = rte_eth_dev_count_total();
> -
> /* set up array for port data */
> mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
> rte_socket_id(), NO_FLAGS);
> @@ -259,7 +256,7 @@ init(int argc, char *argv[])
> ports = mz->addr;
>
> /* parse additional, application arguments */
> - retval = parse_app_args(total_ports, argc, argv);
> + retval = parse_app_args(argc, argv);
> if (retval != 0)
> return -1;
>
> --
> 2.20.1
On Fri, 2 Aug 2019 05:33:20 +0000
Matan Azrad <matan@mellanox.com> wrote:
> Hi Stephen
>
> One more small comment inline
>
> From: Stephen Hemminger
> > Sent: Friday, August 2, 2019 5:58 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [PATCH v5 1/4]
> > examples/multi_process/client_server_mp: check port validity
> >
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> >
> > The mp_server incorrectly allows a port mask that included hidden ports and
> > which later caused either lost packets or failed initialization.
> >
> > This fixes explicitly checking that each bit in portmask is a valid port before
> > using it.
> >
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> > .../client_server_mp/mp_server/args.c | 35 ++++++++++---------
> > .../client_server_mp/mp_server/args.h | 2 +-
> > .../client_server_mp/mp_server/init.c | 7 ++--
> > 3 files changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/examples/multi_process/client_server_mp/mp_server/args.c
> > b/examples/multi_process/client_server_mp/mp_server/args.c
> > index b0d8d7665c85..fdc008b3d677 100644
> > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > @@ -10,6 +10,7 @@
> > #include <errno.h>
> >
> > #include <rte_memory.h>
> > +#include <rte_ethdev.h>
> > #include <rte_string_fns.h>
> >
> > #include "common.h"
> > @@ -41,31 +42,33 @@ usage(void)
> > * array variable
> > */
> > static int
> > -parse_portmask(uint8_t max_ports, const char *portmask)
> > +parse_portmask(const char *portmask)
> > {
> > char *end = NULL;
> > unsigned long pm;
> > - uint16_t count = 0;
> > + uint16_t id;
> >
> > if (portmask == NULL || *portmask == '\0')
> > return -1;
> >
> > /* convert parameter to a number and verify */
> > pm = strtoul(portmask, &end, 16);
> > - if (end == NULL || *end != '\0' || pm == 0)
> > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
>
> Why pm > UINT16_MAX ? should be something like > (1 << RTE_MAX_ETHPORTS) - 1.
> And need to be sure pm type can hold RTE_MAX_ETHPORTS bits, otherwise port 0 may unlikely be all the time visible in the loop below.
>
The DPDK assumes a lot of places that unsigned long will hold a port mask.
If some extra bits are set, the error is visible later when the bits are leftover
after finding ports.
The original code had worse problems, it would not catch invalid pm values at all
and truncate silently.
Hi Stephen
From: Stephen Hemminger
> On Fri, 2 Aug 2019 05:33:20 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > Hi Stephen
> >
> > One more small comment inline
> >
> > From: Stephen Hemminger
> > > Sent: Friday, August 2, 2019 5:58 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > Subject: [dpdk-dev] [PATCH v5 1/4]
> > > examples/multi_process/client_server_mp: check port validity
> > >
> > > From: Stephen Hemminger <sthemmin@microsoft.com>
> > >
> > > The mp_server incorrectly allows a port mask that included hidden
> > > ports and which later caused either lost packets or failed initialization.
> > >
> > > This fixes explicitly checking that each bit in portmask is a valid
> > > port before using it.
> > >
> > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > ---
> > > .../client_server_mp/mp_server/args.c | 35 ++++++++++---------
> > > .../client_server_mp/mp_server/args.h | 2 +-
> > > .../client_server_mp/mp_server/init.c | 7 ++--
> > > 3 files changed, 22 insertions(+), 22 deletions(-)
> > >
> > > diff --git
> > > a/examples/multi_process/client_server_mp/mp_server/args.c
> > > b/examples/multi_process/client_server_mp/mp_server/args.c
> > > index b0d8d7665c85..fdc008b3d677 100644
> > > --- a/examples/multi_process/client_server_mp/mp_server/args.c
> > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c
> > > @@ -10,6 +10,7 @@
> > > #include <errno.h>
> > >
> > > #include <rte_memory.h>
> > > +#include <rte_ethdev.h>
> > > #include <rte_string_fns.h>
> > >
> > > #include "common.h"
> > > @@ -41,31 +42,33 @@ usage(void)
> > > * array variable
> > > */
> > > static int
> > > -parse_portmask(uint8_t max_ports, const char *portmask)
> > > +parse_portmask(const char *portmask)
> > > {
> > > char *end = NULL;
> > > unsigned long pm;
> > > - uint16_t count = 0;
> > > + uint16_t id;
> > >
> > > if (portmask == NULL || *portmask == '\0')
> > > return -1;
> > >
> > > /* convert parameter to a number and verify */
> > > pm = strtoul(portmask, &end, 16);
> > > - if (end == NULL || *end != '\0' || pm == 0)
> > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> >
> > Why pm > UINT16_MAX ? should be something like > (1 <<
> RTE_MAX_ETHPORTS) - 1.
> > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> otherwise port 0 may unlikely be all the time visible in the loop below.
> >
>
> The DPDK assumes a lot of places that unsigned long will hold a port mask.
So, all are bugs, no?
> If some extra bits are set, the error is visible later when the bits are leftover
> after finding ports.
Yes, but if there is a valid port which its port id is bigger than the portmask bits number - port 0 will be all the time visible in the check -> bug.
> The original code had worse problems, it would not catch invalid pm values at
> all and truncate silently.
Yes, maybe, but I really don't understand why you chose to limit for 16 ports, where this number come from?
So, my approach here, 2 options:
1. Remove this line change at all.
2. Do the portmask check bug-free.
Matan
On Sun, 4 Aug 2019 08:31:54 +0000
Matan Azrad <matan@mellanox.com> wrote:
> > > > /* convert parameter to a number and verify */
> > > > pm = strtoul(portmask, &end, 16);
> > > > - if (end == NULL || *end != '\0' || pm == 0)
> > > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
> > >
> > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > RTE_MAX_ETHPORTS) - 1.
> > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > otherwise port 0 may unlikely be all the time visible in the loop below.
> > >
> >
> > The DPDK assumes a lot of places that unsigned long will hold a port mask.
>
> So, all are bugs, no?
I don't think 32 bit build is that well tested. But yes a mask
needs to hold 64 ports.
> > If some extra bits are set, the error is visible later when the bits are leftover
> > after finding ports.
>
> Yes, but if there is a valid port which its port id is bigger than the portmask bits number - port 0 will be all the time visible in the check -> bug.
>
> > The original code had worse problems, it would not catch invalid pm values at
> > all and truncate silently.
>
> Yes, maybe, but I really don't understand why you chose to limit for 16 ports, where this number come from?
> So, my approach here, 2 options:
The problem here was my mistake for not having wide enough portmask.
From: Stephen Hemminger
> On Sun, 4 Aug 2019 08:31:54 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > > > > /* convert parameter to a number and verify */
> > > > > pm = strtoul(portmask, &end, 16);
> > > > > - if (end == NULL || *end != '\0' || pm == 0)
> > > > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm
> == 0)
> > > >
> > > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > > RTE_MAX_ETHPORTS) - 1.
> > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > > otherwise port 0 may unlikely be all the time visible in the loop below.
> > > >
> > >
> > > The DPDK assumes a lot of places that unsigned long will hold a port
> mask.
> >
> > So, all are bugs, no?
>
> I don't think 32 bit build is that well tested. But yes a mask needs to hold 64
> ports.
What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in config file?
Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid port in range [64, 127].
Then, assume the failsafe sub device owns port ID 0.
Because the mask bits are not enough to handle the above range, you will get port 0 as valid port - bug.
I think you need one more check to the RTE_MAX_ETHPORTS > 64 case.
> > > If some extra bits are set, the error is visible later when the bits
> > > are leftover after finding ports.
> >
> > Yes, but if there is a valid port which its port id is bigger than the portmask
> bits number - port 0 will be all the time visible in the check -> bug.
> >
> > > The original code had worse problems, it would not catch invalid pm
> > > values at all and truncate silently.
> >
> > Yes, maybe, but I really don't understand why you chose to limit for 16
> ports, where this number come from?
> > So, my approach here, 2 options:
>
> The problem here was my mistake for not having wide enough portmask.
On Tue, 6 Aug 2019 08:19:01 +0000
Matan Azrad <matan@mellanox.com> wrote:
> From: Stephen Hemminger
> > On Sun, 4 Aug 2019 08:31:54 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >
> > > > > > /* convert parameter to a number and verify */
> > > > > > pm = strtoul(portmask, &end, 16);
> > > > > > - if (end == NULL || *end != '\0' || pm == 0)
> > > > > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm
> > == 0)
> > > > >
> > > > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > > > RTE_MAX_ETHPORTS) - 1.
> > > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > > > otherwise port 0 may unlikely be all the time visible in the loop below.
> > > > >
> > > >
> > > > The DPDK assumes a lot of places that unsigned long will hold a port
> > mask.
> > >
> > > So, all are bugs, no?
> >
> > I don't think 32 bit build is that well tested. But yes a mask needs to hold 64
> > ports.
>
> What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in config file?
>
> Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid port in range [64, 127].
> Then, assume the failsafe sub device owns port ID 0.
>
> Because the mask bits are not enough to handle the above range, you will get port 0 as valid port - bug.
>
> I think you need one more check to the RTE_MAX_ETHPORTS > 64 case.
Not really needed.
The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
Examples include testpmd/parameters.c etc.
The original concept of a small set of assigned values for portid is not going
to scale. It really should have been more like ifindex; something that is not
used by common API's much larger range; and assigned purely sequentially.
The API's should all be using names, but the DPDK port naming is also a mess...
Hi
From: Stephen Hemminger
> Sent: Tuesday, August 6, 2019 6:40 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
>
> On Tue, 6 Aug 2019 08:19:01 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > From: Stephen Hemminger
> > > On Sun, 4 Aug 2019 08:31:54 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > > > > /* convert parameter to a number and verify */
> > > > > > > pm = strtoul(portmask, &end, 16);
> > > > > > > - if (end == NULL || *end != '\0' || pm == 0)
> > > > > > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm
> > > == 0)
> > > > > >
> > > > > > Why pm > UINT16_MAX ? should be something like > (1 <<
> > > > > RTE_MAX_ETHPORTS) - 1.
> > > > > > And need to be sure pm type can hold RTE_MAX_ETHPORTS bits,
> > > > > otherwise port 0 may unlikely be all the time visible in the loop below.
> > > > > >
> > > > >
> > > > > The DPDK assumes a lot of places that unsigned long will hold a
> > > > > port
> > > mask.
> > > >
> > > > So, all are bugs, no?
> > >
> > > I don't think 32 bit build is that well tested. But yes a mask needs
> > > to hold 64 ports.
> >
> > What if someone changes RTE_MAX_ETHPORTS to be bigger than 64 in
> config file?
> >
> > Assume the user changes RTE_MAX_ETHPORTS to 128, and there is a valid
> port in range [64, 127].
> > Then, assume the failsafe sub device owns port ID 0.
> >
> > Because the mask bits are not enough to handle the above range, you will
> get port 0 as valid port - bug.
> >
> > I think you need one more check to the RTE_MAX_ETHPORTS > 64 case.
>
> Not really needed.
>
> The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> Examples include testpmd/parameters.c etc.
Yes, I understand, but the user should know not to change the default value of
RTE_MAX_ETHPORTS, at least it should be documented.
> The original concept of a small set of assigned values for portid is not going to
> scale. It really should have been more like ifindex; something that is not used
> by common API's much larger range; and assigned purely sequentially.
>
> The API's should all be using names, but the DPDK port naming is also a
> mess...
Port ID is OK, user can run port info, then to find the wanted port ID and configure it by port id list\bitmap.
On Tue, 6 Aug 2019 20:03:22 +0000
Matan Azrad <matan@mellanox.com> wrote:
> >
> > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > Examples include testpmd/parameters.c etc.
>
> Yes, I understand, but the user should know not to change the default value of
> RTE_MAX_ETHPORTS, at least it should be documented.
>
> > The original concept of a small set of assigned values for portid is not going to
> > scale. It really should have been more like ifindex; something that is not used
> > by common API's much larger range; and assigned purely sequentially.
> >
> > The API's should all be using names, but the DPDK port naming is also a
> > mess...
>
> Port ID is OK, user can run port info, then to find the wanted port ID and configure it by port id list\bitmap.
>
The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will break
lots of other places. Why put more checks in the examples. Sorry, it really
would not help to pretend that fixing the example is going to help this.
The original code was broken with behavior introduced with port ownership
and these patches try to help fix that.
Still think Port ID is a lousy API for real applications. It is fine for the
DPDK literati but terrible user experience for the average user.
It requires too hands on an experience. For example, the port id values change in
non-obvious ways when using port ownership like failsafe does.
From: Stephen Hemminger
> Sent: Wednesday, August 7, 2019 2:09 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> examples/multi_process/client_server_mp: check port validity
>
> On Tue, 6 Aug 2019 20:03:22 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > >
> > > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > > Examples include testpmd/parameters.c etc.
> >
> > Yes, I understand, but the user should know not to change the default
> > value of RTE_MAX_ETHPORTS, at least it should be documented.
> >
> > > The original concept of a small set of assigned values for portid is
> > > not going to scale. It really should have been more like ifindex;
> > > something that is not used by common API's much larger range; and
> assigned purely sequentially.
> > >
> > > The API's should all be using names, but the DPDK port naming is
> > > also a mess...
> >
> > Port ID is OK, user can run port info, then to find the wanted port ID and
> configure it by port id list\bitmap.
> >
>
>
> The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will
> break lots of other places. Why put more checks in the examples. Sorry, it
> really would not help to pretend that fixing the example is going to help this.
Agree that it is not needed to fix all the places now.
It is better just to update the example documentation that RTE_MAX_ETHPORTS must not be changed when running this application.
I will ack your series(v7) , Consider to update the doc if you want to be completely perfect here.
On Wed, 7 Aug 2019 05:38:42 +0000
Matan Azrad <matan@mellanox.com> wrote:
> From: Stephen Hemminger
> > Sent: Wednesday, August 7, 2019 2:09 AM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > examples/multi_process/client_server_mp: check port validity
> >
> > On Tue, 6 Aug 2019 20:03:22 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >
> > > >
> > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64 bits.
> > > > Examples include testpmd/parameters.c etc.
> > >
> > > Yes, I understand, but the user should know not to change the default
> > > value of RTE_MAX_ETHPORTS, at least it should be documented.
> > >
> > > > The original concept of a small set of assigned values for portid is
> > > > not going to scale. It really should have been more like ifindex;
> > > > something that is not used by common API's much larger range; and
> > assigned purely sequentially.
> > > >
> > > > The API's should all be using names, but the DPDK port naming is
> > > > also a mess...
> > >
> > > Port ID is OK, user can run port info, then to find the wanted port ID and
> > configure it by port id list\bitmap.
> > >
> >
> >
> > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it will
> > break lots of other places. Why put more checks in the examples. Sorry, it
> > really would not help to pretend that fixing the example is going to help this.
>
>
> Agree that it is not needed to fix all the places now.
> It is better just to update the example documentation that RTE_MAX_ETHPORTS must not be changed when running this application.
>
> I will ack your series(v7) , Consider to update the doc if you want to be completely perfect here.
Perhaps the right place to tell the users is somewhere in the documentation?
One place would be here:
diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst
index f19c1389b6af..a847d9ceda22 100644
--- a/doc/guides/faq/faq.rst
+++ b/doc/guides/faq/faq.rst
@@ -195,3 +195,8 @@ Why can't my application receive packets on my system with UEFI Secure Boot enab
If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO on the system.
Therefore, devices for use by DPDK should be bound to the ``vfio-pci`` kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
+
+What is the maximum number of ethernet devices?
+-----------------------------------------------
+
+The limit on the number of Ethernet devices is controlled by the RTE_MAX_ETHPORTS configuration setting. Since many of the applications use a 64 bit value for port mask; the current upper limit is 64 ports.
From: Stephen Hemminger
> On Wed, 7 Aug 2019 05:38:42 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > From: Stephen Hemminger
> > > Sent: Wednesday, August 7, 2019 2:09 AM
> > > To: Matan Azrad <matan@mellanox.com>
> > > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > > examples/multi_process/client_server_mp: check port validity
> > >
> > > On Tue, 6 Aug 2019 20:03:22 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > >
> > > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64
> bits.
> > > > > Examples include testpmd/parameters.c etc.
> > > >
> > > > Yes, I understand, but the user should know not to change the
> > > > default value of RTE_MAX_ETHPORTS, at least it should be
> documented.
> > > >
> > > > > The original concept of a small set of assigned values for
> > > > > portid is not going to scale. It really should have been more
> > > > > like ifindex; something that is not used by common API's much
> > > > > larger range; and
> > > assigned purely sequentially.
> > > > >
> > > > > The API's should all be using names, but the DPDK port naming is
> > > > > also a mess...
> > > >
> > > > Port ID is OK, user can run port info, then to find the wanted
> > > > port ID and
> > > configure it by port id list\bitmap.
> > > >
> > >
> > >
> > > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it
> > > will break lots of other places. Why put more checks in the
> > > examples. Sorry, it really would not help to pretend that fixing the
> example is going to help this.
> >
> >
> > Agree that it is not needed to fix all the places now.
> > It is better just to update the example documentation that
> RTE_MAX_ETHPORTS must not be changed when running this application.
> >
> > I will ack your series(v7) , Consider to update the doc if you want to be
> completely perfect here.
>
> Perhaps the right place to tell the users is somewhere in the documentation?
>
> One place would be here:
>
> diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst index
> f19c1389b6af..a847d9ceda22 100644
> --- a/doc/guides/faq/faq.rst
> +++ b/doc/guides/faq/faq.rst
> @@ -195,3 +195,8 @@ Why can't my application receive packets on my
> system with UEFI Secure Boot enab
>
> If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO
> on the system.
> Therefore, devices for use by DPDK should be bound to the ``vfio-pci``
> kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
> +
> +What is the maximum number of ethernet devices?
> +-----------------------------------------------
> +
> +The limit on the number of Ethernet devices is controlled by the
> RTE_MAX_ETHPORTS configuration setting. Since many of the applications
> use a 64 bit value for port mask; the current upper limit is 64 ports.
>
I think there are systems with a lot of virtual ports which may use more than 64.
So update all the docs when the mask is defined, would be option too.
On Wed, 7 Aug 2019 07:02:02 +0000
Matan Azrad <matan@mellanox.com> wrote:
> From: Stephen Hemminger
> > On Wed, 7 Aug 2019 05:38:42 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >
> > > From: Stephen Hemminger
> > > > Sent: Wednesday, August 7, 2019 2:09 AM
> > > > To: Matan Azrad <matan@mellanox.com>
> > > > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v5 1/4]
> > > > examples/multi_process/client_server_mp: check port validity
> > > >
> > > > On Tue, 6 Aug 2019 20:03:22 +0000
> > > > Matan Azrad <matan@mellanox.com> wrote:
> > > >
> > > > > >
> > > > > > The DPDK has lots of hard coded assumptions of all ports fitting in 64
> > bits.
> > > > > > Examples include testpmd/parameters.c etc.
> > > > >
> > > > > Yes, I understand, but the user should know not to change the
> > > > > default value of RTE_MAX_ETHPORTS, at least it should be
> > documented.
> > > > >
> > > > > > The original concept of a small set of assigned values for
> > > > > > portid is not going to scale. It really should have been more
> > > > > > like ifindex; something that is not used by common API's much
> > > > > > larger range; and
> > > > assigned purely sequentially.
> > > > > >
> > > > > > The API's should all be using names, but the DPDK port naming is
> > > > > > also a mess...
> > > > >
> > > > > Port ID is OK, user can run port info, then to find the wanted
> > > > > port ID and
> > > > configure it by port id list\bitmap.
> > > > >
> > > >
> > > >
> > > > The examples are toy programs. If user changes RTE_MAX_ETHPORTS it
> > > > will break lots of other places. Why put more checks in the
> > > > examples. Sorry, it really would not help to pretend that fixing the
> > example is going to help this.
> > >
> > >
> > > Agree that it is not needed to fix all the places now.
> > > It is better just to update the example documentation that
> > RTE_MAX_ETHPORTS must not be changed when running this application.
> > >
> > > I will ack your series(v7) , Consider to update the doc if you want to be
> > completely perfect here.
> >
> > Perhaps the right place to tell the users is somewhere in the documentation?
> >
> > One place would be here:
> >
> > diff --git a/doc/guides/faq/faq.rst b/doc/guides/faq/faq.rst index
> > f19c1389b6af..a847d9ceda22 100644
> > --- a/doc/guides/faq/faq.rst
> > +++ b/doc/guides/faq/faq.rst
> > @@ -195,3 +195,8 @@ Why can't my application receive packets on my
> > system with UEFI Secure Boot enab
> >
> > If UEFI secure boot is enabled, the Linux kernel may disallow the use of UIO
> > on the system.
> > Therefore, devices for use by DPDK should be bound to the ``vfio-pci``
> > kernel module rather than ``igb_uio`` or ``uio_pci_generic``.
> > +
> > +What is the maximum number of ethernet devices?
> > +-----------------------------------------------
> > +
> > +The limit on the number of Ethernet devices is controlled by the
> > RTE_MAX_ETHPORTS configuration setting. Since many of the applications
> > use a 64 bit value for port mask; the current upper limit is 64 ports.
> >
> I think there are systems with a lot of virtual ports which may use more than 64.
>
> So update all the docs when the mask is defined, would be option too.
It would be good if (ie someone should do it but I don't have time);
to have a new type "port_set" which is a variable length bit mask
It could be backwards compatible with existing usage.
Something like existing cpuset command format.
Examples of the Mask Format:
00000001 # just bit 0 set
40000000,00000000,00000000 # just bit 94 set
00000001,00000000,00000000 # just bit 64 set
000000ff,00000000 # bits 32-39 set
00000000,000e3862 # 1,5,6,11-13,17-19 set
@@ -10,6 +10,7 @@
#include <errno.h>
#include <rte_memory.h>
+#include <rte_ethdev.h>
#include <rte_string_fns.h>
#include "common.h"
@@ -41,31 +42,33 @@ usage(void)
* array variable
*/
static int
-parse_portmask(uint8_t max_ports, const char *portmask)
+parse_portmask(const char *portmask)
{
char *end = NULL;
unsigned long pm;
- uint16_t count = 0;
+ uint16_t id;
if (portmask == NULL || *portmask == '\0')
return -1;
/* convert parameter to a number and verify */
pm = strtoul(portmask, &end, 16);
- if (end == NULL || *end != '\0' || pm == 0)
+ if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0)
return -1;
- /* loop through bits of the mask and mark ports */
- while (pm != 0){
- if (pm & 0x01){ /* bit is set in mask, use port */
- if (count >= max_ports)
- printf("WARNING: requested port %u not present"
- " - ignoring\n", (unsigned)count);
- else
- ports->id[ports->num_ports++] = count;
- }
- pm = (pm >> 1);
- count++;
+ RTE_ETH_FOREACH_DEV(id) {
+ unsigned long msk = 1u << id;
+
+ if ((pm & msk) == 0)
+ continue;
+
+ pm &= ~msk;
+ ports->id[ports->num_ports++] = id;
+ }
+
+ if (pm != 0) {
+ printf("WARNING: leftover ports in mask %#lx - ignoring\n",
+ pm);
}
return 0;
@@ -99,7 +102,7 @@ parse_num_clients(const char *clients)
* on error.
*/
int
-parse_app_args(uint16_t max_ports, int argc, char *argv[])
+parse_app_args(int argc, char *argv[])
{
int option_index, opt;
char **argvopt = argv;
@@ -112,7 +115,7 @@ parse_app_args(uint16_t max_ports, int argc, char *argv[])
&option_index)) != EOF){
switch (opt){
case 'p':
- if (parse_portmask(max_ports, optarg) != 0){
+ if (parse_portmask(optarg) != 0) {
usage();
return -1;
}
@@ -5,6 +5,6 @@
#ifndef _ARGS_H_
#define _ARGS_H_
-int parse_app_args(uint16_t max_ports, int argc, char *argv[]);
+int parse_app_args(int argc, char *argv[]);
#endif /* ifndef _ARGS_H_ */
@@ -238,7 +238,7 @@ init(int argc, char *argv[])
{
int retval;
const struct rte_memzone *mz;
- uint16_t i, total_ports;
+ uint16_t i;
/* init EAL, parsing EAL args */
retval = rte_eal_init(argc, argv);
@@ -247,9 +247,6 @@ init(int argc, char *argv[])
argc -= retval;
argv += retval;
- /* get total number of ports */
- total_ports = rte_eth_dev_count_total();
-
/* set up array for port data */
mz = rte_memzone_reserve(MZ_PORT_INFO, sizeof(*ports),
rte_socket_id(), NO_FLAGS);
@@ -259,7 +256,7 @@ init(int argc, char *argv[])
ports = mz->addr;
/* parse additional, application arguments */
- retval = parse_app_args(total_ports, argc, argv);
+ retval = parse_app_args(argc, argv);
if (retval != 0)
return -1;