[v7] app/testpmd: add portlist option
Checks
Commit Message
In current version, we are setting the ports
using portmask. With portmask, we can use only
upto 64 ports. This portlist option enables the user
to use more than 64 ports.
Now we can specify the ports in 2 different ways
- Using portmask (-p [0x]nnn): mask must be in hex format
- Using portlist in the following format
--portlist <p1>[-p2][,p3[-p4],...]
--portmask 0x2 is same as --portlist 1
--portmask 0x3 is same as --portlist 0-1
Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v7:
moved the port validation outside the parser function.
added meaningful comments describing the new functionality.
renamed the variables with meaningful names
v6:
optimized the code to check for duplicates
v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function
v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values
v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser
v2:
moved the parser function to testpmd
---
app/test-pmd/config.c | 114 ++++++++++++++++++++++++++++++++++
app/test-pmd/parameters.c | 5 ++
app/test-pmd/testpmd.h | 3 +
doc/guides/testpmd_app_ug/run_app.rst | 7 +++
4 files changed, 129 insertions(+)
Comments
On 10-Feb-20 5:19 PM, Hariprasad Govindharajan wrote:
> In current version, we are setting the ports
> using portmask. With portmask, we can use only
> upto 64 ports. This portlist option enables the user
> to use more than 64 ports.
> Now we can specify the ports in 2 different ways
> - Using portmask (-p [0x]nnn): mask must be in hex format
> - Using portlist in the following format
> --portlist <p1>[-p2][,p3[-p4],...]
>
> --portmask 0x2 is same as --portlist 1
> --portmask 0x3 is same as --portlist 0-1
>
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---
> v7:
> moved the port validation outside the parser function.
> added meaningful comments describing the new functionality.
> renamed the variables with meaningful names
>
> v6:
> optimized the code to check for duplicates
>
> v5:
> added a check to validate the ports available before
> setting them. also added comments in the testpmd file
> for the new function
>
> v4:
> the parser is modified so that we don't ues 2 arrays
> to convert the listed port values
>
> v3:
> squashed the 2 patches and made it 1 patch with
> changes only in testpmd. Also working on optmizing
> the parser
>
> v2:
> moved the parser function to testpmd
> ---
> app/test-pmd/config.c | 114 ++++++++++++++++++++++++++++++++++
> app/test-pmd/parameters.c | 5 ++
> app/test-pmd/testpmd.h | 3 +
> doc/guides/testpmd_app_ug/run_app.rst | 7 +++
> 4 files changed, 129 insertions(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 9669cbd..962984b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
> }
> }
>
> +/**
> + * Parse the user input and obtain the list of forwarding ports
> + *
> + * @param[in] list
> + * String containing the user input. User can specify
> + * in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + * For example, if the user wants to use all the available
> + * 4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + * If the user wants to use only the ports 1,2 then the input
> + * is 1,2.
> + * valid characters are '-' and ','
> + * invalid chars like '.' or '#' will result in
> + * EAL: Error - exiting with code: 1
> + * Cause: Invalid fwd port list
> + * @param[out] values
> + * This array will be filled with a list of port IDs
> + * based on the user input
> + * Note that duplicate entries are discarded and only the first
> + * count entries in this array are port IDs and all the rest
> + * will contain default values
> + * @param[in] maxsize
> + * This parameter denotes 2 things
> + * 1) Size of the values array
I believe you meant "number", not "size".
> + * 2) Maximum value of each element in the values array
> + * @return
> + * -On success, returns total count of port IDs
> + * -On failure, returns -1.
> + */
> +static int
> +parse_port_list(const char *list, unsigned int *values, int maxsize)
> +{
> + int count = 0;
> + char *end = NULL;
> + int min, max;
> + int value, i;
> + unsigned int marked[maxsize];
> +
> + for (i = 0; i < maxsize; i++)
> + marked[i] = 0;
Wouldn't marked[maxsize] = {0}; work the same?
> +
> + if (list == NULL || values == NULL || maxsize < 0)
> + return -1;
You're checking if maxsize can be negative. First of all, you've already
allocated the array with negative size by this time (the
"marked[maxsize]" one), second, why allow negative values at all? Why
not just make it unsigned?
> +
> + /* Remove all blank characters ahead */
> + while (isblank(*list))
> + list++;
Why do it here when you do this first thing in the do..while loop anyway?
> +
> + min = maxsize;
You're overwriting this value regardless. Why not 0? If you want to know
for sure that the value either has or has not been modified, the
conventional way to do this is to use INT_MAX from <limits.h>.
> +
> + do {
> + while (isblank(*list))
> + list++;
I have a suspicion that isblank() will not return 'true' on '\0' so
there's probably a buffer overrun here, if you try to dereference *list
while going past '\0'.
> + if (*list == '\0')
> + return -1;
> + errno = 0;
> + value = strtol(list, &end, 10);
> + if (errno || end == NULL)
> + return -1;
> + if (value < 0 || value >= maxsize)
> + return -1;
> + while (isblank(*end))
> + end++;
> + if (*end == '-') {
> + min = value;
> + } else if ((*end == ',') || (*end == '\0')) {
> + max = value;
> + if (min == maxsize)
> + min = value;
> + for (i = min; i <= max; i++) {
> + if (count < maxsize) {
> + if (marked[i])
> + continue;
> + values[count] = i;
> + marked[i] = 1;
> + count++;
> + }
> + }
> + min = maxsize;
Probably clearer to reset both to zero or INT_MAX/INT_MIN?
> + } else
> + return -1;
> + list = end + 1;
> + } while (*end != '\0');
> +
> + if (count == 0)
> + return -1;
> + return count;
> +}
> +
> +void
> +parse_fwd_portlist(const char *portlist)
> +{
> + int portcount;
> + unsigned int portindex[RTE_MAX_ETHPORTS];
> + int i, valid_port_count = 0;
unsigned?
> +
> + portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
> + if (portcount < 0)
> + rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> +
> + /*
> + * Here we verify the validity of the ports
> + * and thereby calculate the total number of
> + * valid ports
> + */
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, February 11, 2020 12:01 PM
> To: Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stephen@networkplumber.org; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option
>
> On 10-Feb-20 5:19 PM, Hariprasad Govindharajan wrote:
> > In current version, we are setting the ports using portmask. With
> > portmask, we can use only upto 64 ports. This portlist option enables
> > the user to use more than 64 ports.
> > Now we can specify the ports in 2 different ways
> > - Using portmask (-p [0x]nnn): mask must be in hex format
> > - Using portlist in the following format
> > --portlist <p1>[-p2][,p3[-p4],...]
> >
> > --portmask 0x2 is same as --portlist 1
> > --portmask 0x3 is same as --portlist 0-1
> >
> > Signed-off-by: Hariprasad Govindharajan
> > <hariprasad.govindharajan@intel.com>
> > ---
> > v7:
> > moved the port validation outside the parser function.
> > added meaningful comments describing the new functionality.
> > renamed the variables with meaningful names
> >
> > v6:
> > optimized the code to check for duplicates
> >
> > v5:
> > added a check to validate the ports available before setting them.
> > also added comments in the testpmd file for the new function
> >
> > v4:
> > the parser is modified so that we don't ues 2 arrays to convert the
> > listed port values
> >
> > v3:
> > squashed the 2 patches and made it 1 patch with changes only in
> > testpmd. Also working on optmizing the parser
> >
> > v2:
> > moved the parser function to testpmd
> > ---
> > app/test-pmd/config.c | 114
> ++++++++++++++++++++++++++++++++++
> > app/test-pmd/parameters.c | 5 ++
> > app/test-pmd/testpmd.h | 3 +
> > doc/guides/testpmd_app_ug/run_app.rst | 7 +++
> > 4 files changed, 129 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 9669cbd..962984b 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist,
> unsigned int nb_pt)
> > }
> > }
> >
> > +/**
> > + * Parse the user input and obtain the list of forwarding ports
> > + *
> > + * @param[in] list
> > + * String containing the user input. User can specify
> > + * in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> > + * For example, if the user wants to use all the available
> > + * 4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> > + * If the user wants to use only the ports 1,2 then the input
> > + * is 1,2.
> > + * valid characters are '-' and ','
> > + * invalid chars like '.' or '#' will result in
> > + * EAL: Error - exiting with code: 1
> > + * Cause: Invalid fwd port list
> > + * @param[out] values
> > + * This array will be filled with a list of port IDs
> > + * based on the user input
> > + * Note that duplicate entries are discarded and only the first
> > + * count entries in this array are port IDs and all the rest
> > + * will contain default values
> > + * @param[in] maxsize
> > + * This parameter denotes 2 things
> > + * 1) Size of the values array
>
> I believe you meant "number", not "size".
[Govindharajan, Hariprasad] Nope. Here I meant to say the maximum size of the values array.
>
> > + * 2) Maximum value of each element in the values array
> > + * @return
> > + * -On success, returns total count of port IDs
> > + * -On failure, returns -1.
> > + */
> > +static int
> > +parse_port_list(const char *list, unsigned int *values, int maxsize)
> > +{
> > + int count = 0;
> > + char *end = NULL;
> > + int min, max;
> > + int value, i;
> > + unsigned int marked[maxsize];
> > +
> > + for (i = 0; i < maxsize; i++)
> > + marked[i] = 0;
>
> Wouldn't marked[maxsize] = {0}; work the same?
[Govindharajan, Hariprasad] Nope. For that to work, the array size should be a constant. Here it is a variable.
>
> > +
> > + if (list == NULL || values == NULL || maxsize < 0)
> > + return -1;
>
> You're checking if maxsize can be negative. First of all, you've already
> allocated the array with negative size by this time (the "marked[maxsize]"
> one), second, why allow negative values at all? Why not just make it
> unsigned?
>
> > +
> > + /* Remove all blank characters ahead */
> > + while (isblank(*list))
> > + list++;
>
> Why do it here when you do this first thing in the do..while loop anyway?
[Govindharajan, Hariprasad] Yes. Removed.
>
> > +
> > + min = maxsize;
>
> You're overwriting this value regardless. Why not 0? If you want to know for
> sure that the value either has or has not been modified, the conventional
> way to do this is to use INT_MAX from <limits.h>.
>
> > +
> > + do {
> > + while (isblank(*list))
> > + list++;
>
> I have a suspicion that isblank() will not return 'true' on '\0' so there's
> probably a buffer overrun here, if you try to dereference *list while going
> past '\0'.
[Govindharajan, Hariprasad] Corrected
>
> > + if (*list == '\0')
> > + return -1;
> > + errno = 0;
> > + value = strtol(list, &end, 10);
> > + if (errno || end == NULL)
> > + return -1;
> > + if (value < 0 || value >= maxsize)
> > + return -1;
> > + while (isblank(*end))
> > + end++;
> > + if (*end == '-') {
> > + min = value;
> > + } else if ((*end == ',') || (*end == '\0')) {
> > + max = value;
> > + if (min == maxsize)
> > + min = value;
> > + for (i = min; i <= max; i++) {
> > + if (count < maxsize) {
> > + if (marked[i])
> > + continue;
> > + values[count] = i;
> > + marked[i] = 1;
> > + count++;
> > + }
> > + }
> > + min = maxsize;
>
> Probably clearer to reset both to zero or INT_MAX/INT_MIN?
[Govindharajan, Hariprasad] done
>
> > + } else
> > + return -1;
> > + list = end + 1;
> > + } while (*end != '\0');
> > +
> > + if (count == 0)
> > + return -1;
> > + return count;
> > +}
> > +
> > +void
> > +parse_fwd_portlist(const char *portlist) {
> > + int portcount;
> > + unsigned int portindex[RTE_MAX_ETHPORTS];
> > + int i, valid_port_count = 0;
>
> unsigned?
[Govindharajan, Hariprasad] Changed. Initially I was comparing those 2 variables with a signed variable,
So declared them as signed as well.
>
> > +
> > + portcount = parse_port_list(portlist, portindex,
> RTE_MAX_ETHPORTS);
> > + if (portcount < 0)
> > + rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> > +
> > + /*
> > + * Here we verify the validity of the ports
> > + * and thereby calculate the total number of
> > + * valid ports
> > + */
>
> --
> Thanks,
> Anatoly
@@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
}
}
+/**
+ * Parse the user input and obtain the list of forwarding ports
+ *
+ * @param[in] list
+ * String containing the user input. User can specify
+ * in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ * For example, if the user wants to use all the available
+ * 4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ * If the user wants to use only the ports 1,2 then the input
+ * is 1,2.
+ * valid characters are '-' and ','
+ * invalid chars like '.' or '#' will result in
+ * EAL: Error - exiting with code: 1
+ * Cause: Invalid fwd port list
+ * @param[out] values
+ * This array will be filled with a list of port IDs
+ * based on the user input
+ * Note that duplicate entries are discarded and only the first
+ * count entries in this array are port IDs and all the rest
+ * will contain default values
+ * @param[in] maxsize
+ * This parameter denotes 2 things
+ * 1) Size of the values array
+ * 2) Maximum value of each element in the values array
+ * @return
+ * -On success, returns total count of port IDs
+ * -On failure, returns -1.
+ */
+static int
+parse_port_list(const char *list, unsigned int *values, int maxsize)
+{
+ int count = 0;
+ char *end = NULL;
+ int min, max;
+ int value, i;
+ unsigned int marked[maxsize];
+
+ for (i = 0; i < maxsize; i++)
+ marked[i] = 0;
+
+ if (list == NULL || values == NULL || maxsize < 0)
+ return -1;
+
+ /* Remove all blank characters ahead */
+ while (isblank(*list))
+ list++;
+
+ min = maxsize;
+
+ do {
+ while (isblank(*list))
+ list++;
+ if (*list == '\0')
+ return -1;
+ errno = 0;
+ value = strtol(list, &end, 10);
+ if (errno || end == NULL)
+ return -1;
+ if (value < 0 || value >= maxsize)
+ return -1;
+ while (isblank(*end))
+ end++;
+ if (*end == '-') {
+ min = value;
+ } else if ((*end == ',') || (*end == '\0')) {
+ max = value;
+ if (min == maxsize)
+ min = value;
+ for (i = min; i <= max; i++) {
+ if (count < maxsize) {
+ if (marked[i])
+ continue;
+ values[count] = i;
+ marked[i] = 1;
+ count++;
+ }
+ }
+ min = maxsize;
+ } else
+ return -1;
+ list = end + 1;
+ } while (*end != '\0');
+
+ if (count == 0)
+ return -1;
+ return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+ int portcount;
+ unsigned int portindex[RTE_MAX_ETHPORTS];
+ int i, valid_port_count = 0;
+
+ portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
+ if (portcount < 0)
+ rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+ /*
+ * Here we verify the validity of the ports
+ * and thereby calculate the total number of
+ * valid ports
+ */
+ for (i = 0; i < portcount && valid_port_count < portcount; i++) {
+ if (rte_eth_dev_is_valid_port(portindex[i])) {
+ portindex[valid_port_count] = portindex[i];
+ valid_port_count++;
+ }
+ }
+
+ set_fwd_ports_list(portindex, valid_port_count);
+}
+
void
set_fwd_ports_mask(uint64_t portmask)
{
@@ -57,6 +57,7 @@ usage(char* progname)
"[--help|-h] | [--auto-start|-a] | ["
"--tx-first | --stats-period=PERIOD | "
"--coremask=COREMASK --portmask=PORTMASK --numa "
+ "--portlist=PORTLIST "
"--mbuf-size= | --total-num-mbufs= | "
"--nb-cores= | --nb-ports= | "
#ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
"packet forwarding.\n");
printf(" --portmask=PORTMASK: hexadecimal bitmask of ports used "
"by the packet forwarding test.\n");
+ printf(" --portlist=PORTLIST: list of forwarding ports\n");
printf(" --numa: enable NUMA-aware allocation of RX/TX rings and of "
"RX memory buffers (mbufs).\n");
printf(" --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
{ "nb-ports", 1, 0, 0 },
{ "coremask", 1, 0, 0 },
{ "portmask", 1, 0, 0 },
+ { "portlist", 1, 0, 0 },
{ "numa", 0, 0, 0 },
{ "no-numa", 0, 0, 0 },
{ "mp-anon", 0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
parse_fwd_coremask(optarg);
if (!strcmp(lgopts[opt_idx].name, "portmask"))
parse_fwd_portmask(optarg);
+ if (!strcmp(lgopts[opt_idx].name, "portlist"))
+ parse_fwd_portlist(optarg);
if (!strcmp(lgopts[opt_idx].name, "no-numa"))
numa_support = 0;
if (!strcmp(lgopts[opt_idx].name, "numa"))
@@ -614,6 +614,9 @@ lcore_num(void)
rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
}
+void
+parse_fwd_portlist(const char *port);
+
static inline struct fwd_lcore *
current_fwd_lcore(void)
{
@@ -77,6 +77,13 @@ The command line options are:
Set the hexadecimal bitmask of the ports used by the packet forwarding test.
+* ``--portlist=X``
+
+ Set the forwarding ports based on the user input used by the packet forwarding test.
+ '-' denotes a range of ports to set including the two specified port IDs
+ ',' separates multiple port values.
+ Possible examples like --portlist=0,1 or --portlist=0-2 or --portlist=0,1-2 etc
+
* ``--numa``
Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers