[dpdk-dev,v4,3/8] net/failsafe: add probed etherdev capture

Message ID 1516265026-6469-4-git-send-email-matan@mellanox.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Matan Azrad Jan. 18, 2018, 8:43 a.m. UTC
  Previous fail-safe code didn't support probed sub-devices capture and
failed when it tried to probe them.

Skip fail-safe sub-device probing when it already was probed.

Signed-off-by: Matan Azrad <matan@mellanox.com>
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 doc/guides/nics/fail_safe.rst           |  5 +++
 drivers/net/failsafe/failsafe_args.c    |  2 -
 drivers/net/failsafe/failsafe_eal.c     | 78 ++++++++++++++++++++++++---------
 drivers/net/failsafe/failsafe_private.h |  2 +
 4 files changed, 65 insertions(+), 22 deletions(-)
  

Comments

Gaëtan Rivet Jan. 18, 2018, 9:10 a.m. UTC | #1
Hi Matan,

On Thu, Jan 18, 2018 at 08:43:41AM +0000, Matan Azrad wrote:
> Previous fail-safe code didn't support probed sub-devices capture and
> failed when it tried to probe them.
> 
> Skip fail-safe sub-device probing when it already was probed.
> 

What happens when

app --vdev "net_failsafe0,dev(net_failsafe0)" -- -i

? I guess infinite recursion.

> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  doc/guides/nics/fail_safe.rst           |  5 +++
>  drivers/net/failsafe/failsafe_args.c    |  2 -
>  drivers/net/failsafe/failsafe_eal.c     | 78 ++++++++++++++++++++++++---------
>  drivers/net/failsafe/failsafe_private.h |  2 +
>  4 files changed, 65 insertions(+), 22 deletions(-)
> 
> diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst
> index 5b1b47e..b89e53b 100644
> --- a/doc/guides/nics/fail_safe.rst
> +++ b/doc/guides/nics/fail_safe.rst
> @@ -115,6 +115,11 @@ Fail-safe command line parameters
>    order to take only the last line into account (unlike ``exec()``) at every
>    probe attempt.
>  
> +.. note::
> +
> +   In case of whitelist sub-device probed by EAL, fail-safe PMD will take the device
> +   as is, which means that EAL device options are taken in this case.
> +

This note should be right under the "dev()" parameter help I think.

If the self-capture is possible and you fix it, you should as well add a line
here about the limitation, concerning the PCI blacklist mode and the
expected PCI id format?

Something like:

--- 8< ---

   When trying to use a PCI device automatically probed in blacklist mode,
   the syntax for the fail-safe must be with the full PCI id:
   Domain:Bus:Device.Function. See the usage example section.

..                                     ^^^^^^^^^^^^^ Here, an ReST reference
..                                                   Would be nice, I don't recall
..                                                   the exact syntax.
.. In the `Usage example` section:

#. Start testpmd, automatically probing the device 84:00.0 and using it with
   the fail-safe

   .. code-block:: console

      $RTE_TARGET/build/app/testpmd -c 0xff -n 4 \
         --vdev 'net_failsafe0,dev(0000:84:00.0),dev(net_ring0)' \
         -- -i

--- >8 ---

Ensure that this is working before using this command, I haven't tested it.

Regards,
  
Matan Azrad Jan. 18, 2018, 9:33 a.m. UTC | #2
Hi Gaetan

From: Gaëtan Rivet, Thursday, January 18, 2018 11:11 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [PATCH v4 3/8] net/failsafe: add probed etherdev capture
> 
> Hi Matan,
> 
> On Thu, Jan 18, 2018 at 08:43:41AM +0000, Matan Azrad wrote:
> > Previous fail-safe code didn't support probed sub-devices capture and
> > failed when it tried to probe them.
> >
> > Skip fail-safe sub-device probing when it already was probed.
> >
> 
> What happens when
> 
> app --vdev "net_failsafe0,dev(net_failsafe0)" -- -i
> 
> ? I guess infinite recursion.
> 

:) interesting

./x86_64-native-linuxapp-gcc/build/app/test-pmd/testpmd -n 4  --vdev="net_failsafe0,dev(net_failsafe0)" --vdev="net_vdev_netvsc,ignore=1" -- --burst=118 --mbcache=512 --portmask 0xf -i  --nb-cores=11  --rxq=2 --txq=2  --txd=1024 --rxd=1024 
EAL: Detected 12 lcore(s)
EAL: No free hugepages reported in hugepages-1048576kB
EAL: Debug dataplane logs available - lower performance
EAL: Probing VFIO support...
EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable clock cycles !
EAL: PCI device 0002:00:02.0 on NUMA socket 0
EAL:   probe driver: 15b3:1004 net_mlx4
PMD: net_mlx4: PCI information matches, using device "mlx4_0" (VF: true)
PMD: net_mlx4: 1 port(s) detected
PMD: net_mlx4: port 1 MAC address is 00:15:5d:44:4b:24
PMD: net_failsafe: Initializing Fail-safe PMD for net_failsafe0
PMD: net_failsafe: Creating fail-safe device on NUMA socket 0
PMD: net_failsafe: Taking control of a probed sub device 0 named net_failsafe0
PMD: net_failsafe: MAC address is 00:00:00:00:00:00
Interactive-mode selected
testpmd: create a new mbuf pool <mbuf_pool_socket_0>: n=327680, size=2176, socket=0
Configuring Port 0 (socket 0)
Port 0: 00:15:5D:44:4B:24
Checking link statuses...
Done
testpmd> 

Failsafe0 took control of itself (since it is already probed we don't probe it again).

> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  doc/guides/nics/fail_safe.rst           |  5 +++
> >  drivers/net/failsafe/failsafe_args.c    |  2 -
> >  drivers/net/failsafe/failsafe_eal.c     | 78 ++++++++++++++++++++++++---
> ------
> >  drivers/net/failsafe/failsafe_private.h |  2 +
> >  4 files changed, 65 insertions(+), 22 deletions(-)
> >
> > diff --git a/doc/guides/nics/fail_safe.rst
> > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > --- a/doc/guides/nics/fail_safe.rst
> > +++ b/doc/guides/nics/fail_safe.rst
> > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> >    order to take only the last line into account (unlike ``exec()``) at every
> >    probe attempt.
> >
> > +.. note::
> > +
> > +   In case of whitelist sub-device probed by EAL, fail-safe PMD will take the
> device
> > +   as is, which means that EAL device options are taken in this case.
> > +
> 
> This note should be right under the "dev()" parameter help I think.
> 
OK.

> If the self-capture is possible and you fix it, you should as well add a line here
> about the limitation, concerning the PCI blacklist mode and the expected PCI
> id format?
> 
> Something like:
> 
> --- 8< ---
> 
>    When trying to use a PCI device automatically probed in blacklist mode,
>    the syntax for the fail-safe must be with the full PCI id:
>    Domain:Bus:Device.Function. See the usage example section.
> 
> ..                                     ^^^^^^^^^^^^^ Here, an ReST reference
> ..                                                   Would be nice, I don't recall
> ..                                                   the exact syntax.
> .. In the `Usage example` section:
> 
> #. Start testpmd, automatically probing the device 84:00.0 and using it with
>    the fail-safe
> 
>    .. code-block:: console
> 
>       $RTE_TARGET/build/app/testpmd -c 0xff -n 4 \
>          --vdev 'net_failsafe0,dev(0000:84:00.0),dev(net_ring0)' \
>          -- -i
> 
> --- >8 ---
> 
Ok.
> Ensure that this is working before using this command, I haven't tested it.
> 
Sure.
> Regards,
> --
> Gaëtan Rivet
> 6WIND
  

Patch

diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst
index 5b1b47e..b89e53b 100644
--- a/doc/guides/nics/fail_safe.rst
+++ b/doc/guides/nics/fail_safe.rst
@@ -115,6 +115,11 @@  Fail-safe command line parameters
   order to take only the last line into account (unlike ``exec()``) at every
   probe attempt.
 
+.. note::
+
+   In case of whitelist sub-device probed by EAL, fail-safe PMD will take the device
+   as is, which means that EAL device options are taken in this case.
+
 - **mac** parameter [MAC address]
 
   This parameter allows the user to set a default MAC address to the fail-safe
diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c
index db5235b..daf5ed0 100644
--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c
@@ -45,8 +45,6 @@ 
 
 #include "failsafe_private.h"
 
-#define DEVARGS_MAXLEN 4096
-
 /* Callback used when a new device is found in devargs */
 typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params,
 		uint8_t head);
diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index 19d26f5..33a5adf 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -36,39 +36,77 @@ 
 #include "failsafe_private.h"
 
 static int
+fs_ethdev_portid_get(const char *name, uint16_t *port_id)
+{
+	uint16_t pid;
+	size_t len;
+
+	if (name == NULL) {
+		DEBUG("Null pointer is specified\n");
+		return -EINVAL;
+	}
+	len = strlen(name);
+	RTE_ETH_FOREACH_DEV(pid) {
+		if (!strncmp(name, rte_eth_devices[pid].device->name, len)) {
+			*port_id = pid;
+			return 0;
+		}
+	}
+	return -ENODEV;
+}
+
+static int
 fs_bus_init(struct rte_eth_dev *dev)
 {
 	struct sub_device *sdev;
 	struct rte_devargs *da;
 	uint8_t i;
-	uint16_t j;
+	uint16_t pid;
 	int ret;
 
 	FOREACH_SUBDEV(sdev, i, dev) {
 		if (sdev->state != DEV_PARSED)
 			continue;
 		da = &sdev->devargs;
-		ret = rte_eal_hotplug_add(da->bus->name,
-					  da->name,
-					  da->args);
-		if (ret) {
-			ERROR("sub_device %d probe failed %s%s%s", i,
-			      rte_errno ? "(" : "",
-			      rte_errno ? strerror(rte_errno) : "",
-			      rte_errno ? ")" : "");
-			continue;
-		}
-		RTE_ETH_FOREACH_DEV(j) {
-			if (strcmp(rte_eth_devices[j].device->name,
-				    da->name) == 0) {
-				ETH(sdev) = &rte_eth_devices[j];
-				break;
+		if (fs_ethdev_portid_get(da->name, &pid) != 0) {
+			ret = rte_eal_hotplug_add(da->bus->name,
+						  da->name,
+						  da->args);
+			if (ret) {
+				ERROR("sub_device %d probe failed %s%s%s", i,
+				      rte_errno ? "(" : "",
+				      rte_errno ? strerror(rte_errno) : "",
+				      rte_errno ? ")" : "");
+				continue;
 			}
+			if (fs_ethdev_portid_get(da->name, &pid) != 0) {
+				ERROR("sub_device %d init went wrong", i);
+				return -ENODEV;
+			}
+		} else {
+			char devstr[DEVARGS_MAXLEN] = "";
+			struct rte_devargs *probed_da =
+					rte_eth_devices[pid].device->devargs;
+
+			/* Take control of device probed by EAL options. */
+			free(da->args);
+			memset(da, 0, sizeof(*da));
+			if (probed_da != NULL)
+				snprintf(devstr, sizeof(devstr), "%s,%s",
+					 probed_da->name, probed_da->args);
+			else
+				snprintf(devstr, sizeof(devstr), "%s",
+					 rte_eth_devices[pid].device->name);
+			ret = rte_eal_devargs_parse(devstr, da);
+			if (ret) {
+				ERROR("Probed devargs parsing failed with code"
+				      " %d", ret);
+				return ret;
+			}
+			INFO("Taking control of a probed sub device"
+			      " %d named %s", i, da->name);
 		}
-		if (ETH(sdev) == NULL) {
-			ERROR("sub_device %d init went wrong", i);
-			return -ENODEV;
-		}
+		ETH(sdev) = &rte_eth_devices[pid];
 		SUB_ID(sdev) = i;
 		sdev->fs_dev = dev;
 		sdev->dev = ETH(sdev)->device;
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 5e04ffe..9fcf72e 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -58,6 +58,8 @@ 
 #define FAILSAFE_MAX_ETHPORTS 2
 #define FAILSAFE_MAX_ETHADDR 128
 
+#define DEVARGS_MAXLEN 4096
+
 /* TYPES */
 
 struct rxq {