[dpdk-dev,1/1] net/mlx4: add port parameter
Checks
Commit Message
Most ConnectX-3 adapters expose two physical ports on a single PCI bus
address.
Add a new port parameter allowing the user to choose
either or both physical ports to be used by the application.
This parameter is used as follows:
Selecting only the second port:
-w 00:00.0,port=1
Selecting both ports:
-w 00:00.0,port=0,port=1
If no parameter is given, the default behavior is unchanged: all ports
are probed.
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
doc/guides/nics/mlx4.rst | 6 +++
drivers/net/mlx4/Makefile | 1 +
drivers/net/mlx4/mlx4.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/net/mlx4/mlx4.h | 6 +++
4 files changed, 117 insertions(+)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
> Sent: Friday, March 03, 2017 10:40 AM
...
> + errno = 0;
> + tmp = strtoul(val, NULL, 0);
The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
char *end = NULL;
tmp = strtoull(val, &end, 0);
if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
> + if (errno) {
> + WARN("%s: \"%s\" is not a valid integer", key, val);
> + return -errno;
> + }
> + if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
> + if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
> + ERROR("invalid port index %lu (max: %u)",
> + tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
> + return -EINVAL;
> + }
> + conf->active_ports |= 1 << tmp;
> + } else {
> + WARN("%s: unknown parameter", key);
> + return -EINVAL;
> + }
> + return 0;
> +}
The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement. That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.
> + if (mlx4_args(pci_dev->device.devargs, &conf)) {
> + ERROR("failed to process device arguments");
> + goto error;
> + }
It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input.
> + /* Use all ports when none are defined */
> + if (conf.active_ports == 0) {
> + for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
> + conf.active_ports |= 1 << i;
> + }
Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this. Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:
if (conf.active_ports & !(conf.active_ports & (1 << i)))
continue;
On Fri, 10 Mar 2017 16:24:32 +0000
"Legacy, Allain" <Allain.Legacy@windriver.com> wrote:
> The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
>
> char *end = NULL;
> tmp = strtoull(val, &end, 0);
> if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
Extra () no necessary here.
Also errno is not set unless the return value is ULLONG_MAX. It will be last
value.
Something like:
tmp = strtoull(val, &end, 0);
if (!*val || !*end || (tmp == ULLONG_MAX && errno))
...
If endptr is not NULL, strtoul() stores the address of the first
invalid character in *endptr. If there were no digits at all, str‐
toul() stores the original value of nptr in *endptr (and returns 0).
In particular, if *nptr is not '\0' but **endptr is '\0' on return, the
entire string is valid.
...
RETURN VALUE
The strtoul() function returns either the result of the conversion or,
if there was a leading minus sign, the negation of the result of the
conversion represented as an unsigned value, unless the original (non‐
negated) value would overflow; in the latter case, strtoul() returns
ULONG_MAX and sets errno to ERANGE. Precisely the same holds for str‐
toull() (with ULLONG_MAX instead of ULONG_MAX).
Hey, thanks for reading.
On Fri, Mar 10, 2017 at 04:24:32PM +0000, Legacy, Allain wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
>> Sent: Friday, March 03, 2017 10:40 AM
>...
>> + errno = 0;
>> + tmp = strtoul(val, NULL, 0);
>The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
>
> char *end = NULL;
> tmp = strtoull(val, &end, 0);
> if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
>
>
Thanks for the suggestion, I'd keep the strtoul though ;).
I will see into it for the v2, keeping in mind Stephen's remarks as
well.
>> + if (errno) {
>> + WARN("%s: \"%s\" is not a valid integer", key, val);
>> + return -errno;
>> + }
>> + if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
>> + if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
>> + ERROR("invalid port index %lu (max: %u)",
>> + tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
>> + return -EINVAL;
>> + }
>> + conf->active_ports |= 1 << tmp;
>> + } else {
>> + WARN("%s: unknown parameter", key);
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement. That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.
>
>
This function was aimed at being generic.
If we consider that no other parameter would ever be added, then the
strcmp should be scraped altogether, as this callback is only called
upon parsing this parameter in the kvlist in the first place.
But we are in the control path, avoiding a useless strtoul at the price
of making the function less useful seems an unnecessary tradeoff to me.
>> + if (mlx4_args(pci_dev->device.devargs, &conf)) {
>> + ERROR("failed to process device arguments");
>> + goto error;
>> + }
>It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input.
>
>
Agreed.
>> + /* Use all ports when none are defined */
>> + if (conf.active_ports == 0) {
>> + for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
>> + conf.active_ports |= 1 << i;
>> + }
>
>Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this. Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:
>
>if (conf.active_ports & !(conf.active_ports & (1 << i)))
> continue;
>
>
I do not agree with removing this loop.
Your second solution will scatter the relevant bits concerning the
default value of the active_port configuration option. While being
slightly slicker it hides it unnecessarily from the reader.
The first solution might be interesting, however it makes this option
dependent on two defines instead of one. If one had to change the
default MAX_PHYS_PORT value for mlx4 (however unlikely it might be),
then they would have to change the valid ALL_PORTS mask as well. In
principle this contradicts DRY[1].
[1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself
slight additional remark below.
On Fri, Mar 10, 2017 at 06:11:59PM +0100, Gaëtan Rivet wrote:
>Hey, thanks for reading.
>
>On Fri, Mar 10, 2017 at 04:24:32PM +0000, Legacy, Allain wrote:
>>>-----Original Message-----
>>>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
>>>Sent: Friday, March 03, 2017 10:40 AM
>>...
>>>+ errno = 0;
>>>+ tmp = strtoul(val, NULL, 0);
>>The robustness of the strtoul() could be improved with something like the following to catch non-integer characters following the port number.
>>
>> char *end = NULL;
>> tmp = strtoull(val, &end, 0);
>> if ((val[0] == '\0') || (end == NULL) || (*end != '\0') || (errno != 0))
>>
>>
>
>Thanks for the suggestion, I'd keep the strtoul though ;).
>I will see into it for the v2, keeping in mind Stephen's remarks as
>well.
>
>>>+ if (errno) {
>>>+ WARN("%s: \"%s\" is not a valid integer", key, val);
>>>+ return -errno;
>>>+ }
>>>+ if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
>>>+ if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
>>>+ ERROR("invalid port index %lu (max: %u)",
>>>+ tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
>>>+ return -EINVAL;
>>>+ }
>>>+ conf->active_ports |= 1 << tmp;
>>>+ } else {
>>>+ WARN("%s: unknown parameter", key);
>>>+ return -EINVAL;
>>>+ }
>>>+ return 0;
>>>+}
>>The usage of strtoul() should be moved to be within the strcmp(MLX4_PMD_PORT_KVARG, key) IF statement. That way the "val" would only be parsed if "key" is "port" and it is expected that "val" is an integer.
>>
>>
>
>This function was aimed at being generic.
>If we consider that no other parameter would ever be added, then the
>strcmp should be scraped altogether, as this callback is only called
>upon parsing this parameter in the kvlist in the first place.
>
>But we are in the control path, avoiding a useless strtoul at the
>price of making the function less useful seems an unnecessary tradeoff
>to me.
>
>>>+ if (mlx4_args(pci_dev->device.devargs, &conf)) {
>>>+ ERROR("failed to process device arguments");
>>>+ goto error;
>>>+ }
>>It would be helpful for debugging if the error message included the devargs so that we can see what is wrong with the input.
>>
>>
>
>Agreed.
>
Actually, on second thought, here the devargs that was problematic has
already been shown with a warning while it was being parsed.
>>>+ /* Use all ports when none are defined */
>>>+ if (conf.active_ports == 0) {
>>>+ for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
>>>+ conf.active_ports |= 1 << i;
>>>+ }
>>
>>Rather than use a loop to populate all active fields would a #define with an all ports mask be better suited to this. Or alternatively just change the IF statement below to use the following and avoid the need for this loop altogether:
>>
>>if (conf.active_ports & !(conf.active_ports & (1 << i)))
>> continue;
>>
>>
>
>I do not agree with removing this loop.
>
>Your second solution will scatter the relevant bits concerning the
>default value of the active_port configuration option. While being
>slightly slicker it hides it unnecessarily from the reader.
>
>The first solution might be interesting, however it makes this option
>dependent on two defines instead of one. If one had to change the
>default MAX_PHYS_PORT value for mlx4 (however unlikely it might be),
>then they would have to change the valid ALL_PORTS mask as well. In
>principle this contradicts DRY[1].
>
>[1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself
>
>--
>Gaëtan Rivet
>6WIND
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Friday, March 10, 2017 12:12 PM
> The first solution might be interesting, however it makes this option
> dependent on two defines instead of one. If one had to change the
> default MAX_PHYS_PORT value for mlx4 (however unlikely it might be),
> then they would have to change the valid ALL_PORTS mask as well. In
> principle this contradicts DRY[1].
>
> [1]: https://en.wikipedia.org/wiki/Don't_repeat_yourself
#define MLX4_PMD_MAX_PHYS_PORTS 2
#define MLX4_PMD_ALL_PHYS_PORTS ((1 << MLX4_PMD_MAX_PHYS_PORTS) - 1)
On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> address.
>
> Add a new port parameter allowing the user to choose
> either or both physical ports to be used by the application.
>
> This parameter is used as follows:
>
> Selecting only the second port:
> -w 00:00.0,port=1
>
> Selecting both ports:
> -w 00:00.0,port=0,port=1
>
> If no parameter is given, the default behavior is unchanged: all ports
> are probed.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
I think this patch is good as is. Whatever value results from users
specifying random characters as argument to the port parameter is their
problem, as long as the resulting value is verified to be within bounds,
it's fine.
I'm not saying that checking all possible failure modes of strtoul() is
useless, just that it seems overkill in this specific case. Using atoi()
without any error checking would have been perfectly fine as well.
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
On 3/16/2017 11:04 AM, Adrien Mazarguil wrote:
> On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
>> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
>> address.
>>
>> Add a new port parameter allowing the user to choose
>> either or both physical ports to be used by the application.
>>
>> This parameter is used as follows:
>>
>> Selecting only the second port:
>> -w 00:00.0,port=1
>>
>> Selecting both ports:
>> -w 00:00.0,port=0,port=1
>>
>> If no parameter is given, the default behavior is unchanged: all ports
>> are probed.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>
> I think this patch is good as is. Whatever value results from users
> specifying random characters as argument to the port parameter is their
> problem, as long as the resulting value is verified to be within bounds,
> it's fine.
>
> I'm not saying that checking all possible failure modes of strtoul() is
> useless, just that it seems overkill in this specific case. Using atoi()
> without any error checking would have been perfectly fine as well.
>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Hi Adrien, Gaetan,
Are all comments addressed for this patch? It looks like discussion is
going on?
Thanks,
ferruh
Hi Ferruh,
On Mon, Mar 20, 2017 at 01:24:36PM +0000, Ferruh Yigit wrote:
> On 3/16/2017 11:04 AM, Adrien Mazarguil wrote:
> > On Fri, Mar 03, 2017 at 04:40:06PM +0100, Gaetan Rivet wrote:
> >> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> >> address.
> >>
> >> Add a new port parameter allowing the user to choose
> >> either or both physical ports to be used by the application.
> >>
> >> This parameter is used as follows:
> >>
> >> Selecting only the second port:
> >> -w 00:00.0,port=1
> >>
> >> Selecting both ports:
> >> -w 00:00.0,port=0,port=1
> >>
> >> If no parameter is given, the default behavior is unchanged: all ports
> >> are probed.
> >>
> >> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >
> > I think this patch is good as is. Whatever value results from users
> > specifying random characters as argument to the port parameter is their
> > problem, as long as the resulting value is verified to be within bounds,
> > it's fine.
> >
> > I'm not saying that checking all possible failure modes of strtoul() is
> > useless, just that it seems overkill in this specific case. Using atoi()
> > without any error checking would have been perfectly fine as well.
> >
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>
> Hi Adrien, Gaetan,
>
> Are all comments addressed for this patch? It looks like discussion is
> going on?
I think it's over, no more comments will follow.
On 3/3/2017 3:40 PM, Gaetan Rivet wrote:
> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
> address.
>
> Add a new port parameter allowing the user to choose
> either or both physical ports to be used by the application.
>
> This parameter is used as follows:
>
> Selecting only the second port:
> -w 00:00.0,port=1
>
> Selecting both ports:
> -w 00:00.0,port=0,port=1
>
> If no parameter is given, the default behavior is unchanged: all ports
> are probed.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
Hi Gaetan,
Getting following checkpatch warning [1], can you please address it?
Thanks,
ferruh
[1]
WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#156: FILE: drivers/net/mlx4/mlx4.c:5462:
+ unsigned arg_count;
Hi Ferruh,
On Mon, Mar 27, 2017 at 04:08:51PM +0100, Ferruh Yigit wrote:
>On 3/3/2017 3:40 PM, Gaetan Rivet wrote:
>> Most ConnectX-3 adapters expose two physical ports on a single PCI bus
>> address.
>>
>> Add a new port parameter allowing the user to choose
>> either or both physical ports to be used by the application.
>>
>> This parameter is used as follows:
>>
>> Selecting only the second port:
>> -w 00:00.0,port=1
>>
>> Selecting both ports:
>> -w 00:00.0,port=0,port=1
>>
>> If no parameter is given, the default behavior is unchanged: all ports
>> are probed.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>
>Hi Gaetan,
>
>Getting following checkpatch warning [1], can you please address it?
>
Sure, here it is: http://dpdk.org/ml/archives/dev/2017-March/061582.html
Sorry for that, I was checking it with an older checkpatch.pl.
>Thanks,
>ferruh
>
>[1]
>WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
>#156: FILE: drivers/net/mlx4/mlx4.c:5462:
>+ unsigned arg_count;
@@ -162,6 +162,12 @@ Run-time configuration
- **ethtool** operations on related kernel interfaces also affect the PMD.
+- ``port`` parameter [int]
+
+ This parameter provides a physical port to probe and can be specified multiple
+ times for additional ports. All ports are probed by default if left
+ unspecified.
+
Kernel module parameters
~~~~~~~~~~~~~~~~~~~~~~~~
@@ -43,6 +43,7 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_ether
DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mbuf
DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_eal
DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_mempool
+DEPDIRS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += lib/librte_kvargs
# Basic CFLAGS.
CFLAGS += -O3
@@ -83,6 +83,7 @@
#include <rte_alarm.h>
#include <rte_memory.h>
#include <rte_flow.h>
+#include <rte_kvargs.h>
/* Generated configuration header. */
#include "mlx4_autoconf.h"
@@ -125,6 +126,16 @@ struct mlx4_secondary_data {
rte_spinlock_t lock; /* Port configuration lock. */
} mlx4_secondary_data[RTE_MAX_ETHPORTS];
+struct mlx4_conf {
+ uint8_t active_ports;
+};
+
+/* Available parameters list. */
+const char *pmd_mlx4_init_params[] = {
+ MLX4_PMD_PORT_KVARG,
+ NULL,
+};
+
/**
* Check if running as a secondary process.
*
@@ -5396,6 +5407,84 @@ priv_dev_interrupt_handler_install(struct priv *priv, struct rte_eth_dev *dev)
}
}
+/**
+ * Verify and store value for device argument.
+ *
+ * @param[in] key
+ * Key argument to verify.
+ * @param[in] val
+ * Value associated with key.
+ * @param out
+ * User data.
+ *
+ * @return
+ * 0 on success, negative errno value on failure.
+ */
+static int
+mlx4_arg_parse(const char *key, const char *val, void *out)
+{
+ struct mlx4_conf *conf = out;
+ unsigned long tmp;
+
+ errno = 0;
+ tmp = strtoul(val, NULL, 0);
+ if (errno) {
+ WARN("%s: \"%s\" is not a valid integer", key, val);
+ return -errno;
+ }
+ if (strcmp(MLX4_PMD_PORT_KVARG, key) == 0) {
+ if (tmp >= MLX4_PMD_MAX_PHYS_PORTS) {
+ ERROR("invalid port index %lu (max: %u)",
+ tmp, MLX4_PMD_MAX_PHYS_PORTS - 1);
+ return -EINVAL;
+ }
+ conf->active_ports |= 1 << tmp;
+ } else {
+ WARN("%s: unknown parameter", key);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+/**
+ * Parse device parameters.
+ *
+ * @param devargs
+ * Device arguments structure.
+ *
+ * @return
+ * 0 on success, negative errno value on failure.
+ */
+static int
+mlx4_args(struct rte_devargs *devargs, struct mlx4_conf *conf)
+{
+ struct rte_kvargs *kvlist;
+ unsigned arg_count;
+ int ret = 0;
+ int i;
+
+ if (devargs == NULL)
+ return 0;
+ kvlist = rte_kvargs_parse(devargs->args, pmd_mlx4_init_params);
+ if (kvlist == NULL) {
+ ERROR("failed to parse kvargs");
+ return -EINVAL;
+ }
+ /* Process parameters. */
+ for (i = 0; pmd_mlx4_init_params[i]; ++i) {
+ arg_count = rte_kvargs_count(kvlist, MLX4_PMD_PORT_KVARG);
+ while (arg_count-- > 0) {
+ ret = rte_kvargs_process(kvlist, MLX4_PMD_PORT_KVARG,
+ mlx4_arg_parse, conf);
+ if (ret != 0)
+ goto free_kvlist;
+ }
+ }
+free_kvlist:
+ rte_kvargs_free(kvlist);
+ return ret;
+}
+
static struct eth_driver mlx4_driver;
/**
@@ -5420,6 +5509,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
int err = 0;
struct ibv_context *attr_ctx = NULL;
struct ibv_device_attr device_attr;
+ struct mlx4_conf conf = {
+ .active_ports = 0,
+ };
unsigned int vf;
int idx;
int i;
@@ -5490,6 +5582,15 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
goto error;
INFO("%u port(s) detected", device_attr.phys_port_cnt);
+ if (mlx4_args(pci_dev->device.devargs, &conf)) {
+ ERROR("failed to process device arguments");
+ goto error;
+ }
+ /* Use all ports when none are defined */
+ if (conf.active_ports == 0) {
+ for (i = 0; i < MLX4_PMD_MAX_PHYS_PORTS; i++)
+ conf.active_ports |= 1 << i;
+ }
for (i = 0; i < device_attr.phys_port_cnt; i++) {
uint32_t port = i + 1; /* ports are indexed from one */
uint32_t test = (1 << i);
@@ -5503,6 +5604,9 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
#endif /* HAVE_EXP_QUERY_DEVICE */
struct ether_addr mac;
+ /* If port is not active, skip. */
+ if (!(conf.active_ports & (1 << i)))
+ continue;
#ifdef HAVE_EXP_QUERY_DEVICE
exp_device_attr.comp_mask = IBV_EXP_DEVICE_ATTR_EXP_CAP_FLAGS;
#ifdef RSS_SUPPORT
@@ -81,6 +81,9 @@
/* Request send completion once in every 64 sends, might be less. */
#define MLX4_PMD_TX_PER_COMP_REQ 64
+/* Maximum number of physical ports. */
+#define MLX4_PMD_MAX_PHYS_PORTS 2
+
/* Maximum number of Scatter/Gather Elements per Work Request. */
#ifndef MLX4_PMD_SGE_WR_N
#define MLX4_PMD_SGE_WR_N 4
@@ -113,6 +116,9 @@
/* Alarm timeout. */
#define MLX4_ALARM_TIMEOUT_US 100000
+/* Port parameter. */
+#define MLX4_PMD_PORT_KVARG "port"
+
enum {
PCI_VENDOR_ID_MELLANOX = 0x15b3,
};