diff mbox series

net/mlx5: txq_inline_min not set for ConnectX-5 adapters

Message ID 1564612871-162495-1-git-send-email-drc@linux.vnet.ibm.com (mailing list archive)
State Accepted, archived
Delegated to: Raslan Darawsheh
Headers
Series net/mlx5: txq_inline_min not set for ConnectX-5 adapters |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-Compile-Testing success Compile Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

David Christensen July 31, 2019, 10:41 p.m. UTC 
  The function mlx5_set_min_inline() includes a switch() that checks
various PCI device IDs in order to set the txq_inline_min value.  No
value is set when the PCI device ID matches the ConnectX-5 adapters,
resulting in an assert() failure later in the function
mlx5_set_txlimit_params().

This error was encountered on an IBM Power 9 system running RHEL 7.6
w/o Mellanox OFED installed.

Fixes: 38b4b397a57d ("net/mlx5: add Tx configuration and setup")
Cc: viacheslavo@mellanox.com

Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
---
 drivers/net/mlx5/mlx5.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Stephen Hemminger July 31, 2019, 10:55 p.m. UTC | #1 
On Wed, 31 Jul 2019 17:41:11 -0500
David Christensen <drc@linux.vnet.ibm.com> wrote:

> The function mlx5_set_min_inline() includes a switch() that checks
> various PCI device IDs in order to set the txq_inline_min value.  No
> value is set when the PCI device ID matches the ConnectX-5 adapters,
> resulting in an assert() failure later in the function
> mlx5_set_txlimit_params().
> 
> This error was encountered on an IBM Power 9 system running RHEL 7.6
> w/o Mellanox OFED installed.
> 
> Fixes: 38b4b397a57d ("net/mlx5: add Tx configuration and setup")
> Cc: viacheslavo@mellanox.com
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
>  drivers/net/mlx5/mlx5.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 3a345c7..0f61dd5 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1341,6 +1341,7 @@ struct mlx5_dev_spawn_data {
>  		 * and PFC control may be broken, so disable feature.
>  		 */
>  		config->hw_vlan_insert = 0;
> +		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
>  		break;
>  	default:
>  		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;

This is a case where a simple switch fallthrough would have same effect.
Slava Ovsiienko Aug. 1, 2019, 4:32 a.m. UTC | #2 
Hi, David

Yes, thank you for the patch, acked.

The "txq_inline_min" value was not set for ConnectX-5 to 0 as default.
At the TX queue setup time the "txq_inline_min" is checked against MLX5_ARG_UNSET 
and default value 0 is set:

txq_set_params()
...
inlen_mode = (config->txq_inline_min == MLX5_ARG_UNSET) ?
                     0 : (unsigned int)config->txq_inline_min;

So, there should be no negative backwards. Did you observe any?

WBR, Slava

> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Thursday, August 1, 2019 1:41
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; David Christensen <drc@linux.vnet.ibm.com>
> Subject: [PATCH] net/mlx5: txq_inline_min not set for ConnectX-5 adapters
> 
> The function mlx5_set_min_inline() includes a switch() that checks various
> PCI device IDs in order to set the txq_inline_min value.  No value is set when
> the PCI device ID matches the ConnectX-5 adapters, resulting in an assert()
> failure later in the function mlx5_set_txlimit_params().
> 
> This error was encountered on an IBM Power 9 system running RHEL 7.6 w/o
> Mellanox OFED installed.
> 
> Fixes: 38b4b397a57d ("net/mlx5: add Tx configuration and setup")
> Cc: viacheslavo@mellanox.com
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>

Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>


> ---
>  drivers/net/mlx5/mlx5.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 3a345c7..0f61dd5 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1341,6 +1341,7 @@ struct mlx5_dev_spawn_data {
>  		 * and PFC control may be broken, so disable feature.
>  		 */
>  		config->hw_vlan_insert = 0;
> +		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
>  		break;
>  	default:
>  		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
> --
> 1.8.3.1
David Christensen Aug. 1, 2019, 6:24 p.m. UTC | #3 
> Yes, thank you for the patch, acked.
> 
> The "txq_inline_min" value was not set for ConnectX-5 to 0 as default.
> At the TX queue setup time the "txq_inline_min" is checked against MLX5_ARG_UNSET
> and default value 0 is set:
> 
> txq_set_params()
> ...
> inlen_mode = (config->txq_inline_min == MLX5_ARG_UNSET) ?
>                       0 : (unsigned int)config->txq_inline_min;
> 
> So, there should be no negative backwards. Did you observe any?

The gdb session below shows how the value is unchanged after passing 
through mlx5_set_min_inline on my system without the change.


sudo LD_LIBRARY_PATH=/home/davec/src/dpdk/build/lib gdb --args 
/home/davec/src/dpdk/build/app/testpmd  -w 0000:01:00.0 -w 0000:01:00.1 
-l 4,8-79 -n 4 --iova-mode=pa  -- --rxq=4 --rxd=1024 --txq=4 --txd=4096 
--nb-cores=8 -i -a --numa --forward-mode=rxonly
GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-114.el7
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "ppc64le-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/davec/src/dpdk/build/app/testpmd...done.
(gdb) b mlx5_set_min_inline
Breakpoint 1 at 0x10579da0: file 
/home/davec/src/dpdk/drivers/net/mlx5/mlx5.c, line 1251.
(gdb) start
Temporary breakpoint 2 at 0x100a9b38: file 
/home/davec/src/dpdk/app/test-pmd/testpmd.c, line 3234.
Starting program: /home/davec/src/dpdk/build/app/testpmd -w 0000:01:00.0 
-w 0000:01:00.1 -l 4,8-79 -n 4 --iova-mode=pa -- --rxq=4 --rxd=1024 
--txq=4 --txd=4096 --nb-cores=8 -i -a --numa --forward-mode=rxonly
warning: Could not load shared library symbols for linux-vdso64.so.1.
Do you need "set solib-search-path" or "set sysroot"?
warning: Unable to find libthread_db matching inferior's thread library, 
thread debugging will not be available.

Temporary breakpoint 2, main (argc=20, argv=0x7fffffffef68) at 
/home/davec/src/dpdk/app/test-pmd/testpmd.c:3234
3234		signal(SIGINT, signal_handler);
Missing separate debuginfos, use: debuginfo-install 
advance-toolchain-at12.0-runtime-12.0-1.ppc64le 
libibverbs-17.2-3.el7.ppc64le
(gdb) c
Continuing.
EAL: Detected 160 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'PA'
EAL: No available hugepages reported in hugepages-2048kB
EAL: Probing VFIO support...
EAL: PCI device 0000:01:00.0 on NUMA socket 0
EAL:   probe driver: 15b3:1019 net_mlx5
net_mlx5: mlx5.c:1684: mlx5_dev_spawn(): MPLS over GRE/UDP tunnel 
offloading disabled due to old OFED/rdma-core version or firmware 
configuration

Breakpoint 1, mlx5_set_min_inline (spawn=0x7fffffffe500, 
config=0x7fffffffe4a0)
     at /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c:1251
1251		if (config->txq_inline_min != MLX5_ARG_UNSET) {
(gdb) p config->txq_inline_min
$1 = -1
(gdb) n
1268		if (config->hca_attr.eth_net_offloads) {
(gdb) n
1323		switch (spawn->pci_dev->id.device_id) {
(gdb) p/x spawn->pci_dev->id.vendor_id
$2 = 0x15b3
(gdb) p/x spawn->pci_dev->id.device_id
$3 = 0x1019
(gdb) n
1343			config->hw_vlan_insert = 0;
(gdb) n
1344			config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
(gdb) tbreak +1
Temporary breakpoint 3 at 0x1057a0f0: file 
/home/davec/src/dpdk/drivers/net/mlx5/mlx5.c, line 1345.
(gdb) jump +1
Continuing at 0x1057a0f0.

Temporary breakpoint 3, mlx5_set_min_inline (spawn=0x7fffffffe500, 
config=0x7fffffffe4a0)
     at /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c:1345
1345			break;
(gdb) n
1351		DRV_LOG(DEBUG, "min tx inline configured: %d", 
config->txq_inline_min);
(gdb) p config->txq_inline_min
$4 = -1
(gdb)
Slava Ovsiienko Aug. 2, 2019, 3:48 a.m. UTC | #4 
> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Thursday, August 1, 2019 21:24
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/mlx5: txq_inline_min not set for ConnectX-5
> adapters
> 
> > Yes, thank you for the patch, acked.
> >
> > The "txq_inline_min" value was not set for ConnectX-5 to 0 as default.
> > At the TX queue setup time the "txq_inline_min" is checked against
> > MLX5_ARG_UNSET and default value 0 is set:
> >
> > txq_set_params()
> > ...
> > inlen_mode = (config->txq_inline_min == MLX5_ARG_UNSET) ?
> >                       0 : (unsigned int)config->txq_inline_min;
> >
> > So, there should be no negative backwards. Did you observe any?
> 
> The gdb session below shows how the value is unchanged after passing
> through mlx5_set_min_inline on my system without the change.

Yes, mlx5_set_min_inline() does not set txq_inline_min by default.
So, your patch is OK.

I mean the actual inline_len is set to 0 before actual usage in txq_set_params()
routine, so not setting config->txq_inline_min to default zero should not cause
negative backwards.


> 
> 
> sudo LD_LIBRARY_PATH=/home/davec/src/dpdk/build/lib gdb --args
> /home/davec/src/dpdk/build/app/testpmd  -w 0000:01:00.0 -w 0000:01:00.1
> -l 4,8-79 -n 4 --iova-mode=pa  -- --rxq=4 --rxd=1024 --txq=4 --txd=4096
> --nb-cores=8 -i -a --numa --forward-mode=rxonly
> GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-114.el7
> Copyright (C) 2013 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgnu.o
> rg%2Flicenses%2Fgpl.html&amp;data=02%7C01%7Cviacheslavo%40mellano
> x.com%7C8a11bcca3a8e4ebe791a08d716ad8097%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C637002806753488254&amp;sdata=Q8ZfYRC
> 5VmPKcm9FA2ExoMexUrMYnODMbFGi1V6V3gQ%3D&amp;reserved=0>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show
> copying"
> and "show warranty" for details.
> This GDB was configured as "ppc64le-redhat-linux-gnu".
> For bug reporting instructions, please see:
> <https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .gnu.org%2Fsoftware%2Fgdb%2Fbugs%2F&amp;data=02%7C01%7Cviachesl
> avo%40mellanox.com%7C8a11bcca3a8e4ebe791a08d716ad8097%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637002806753488254&amp
> ;sdata=YIxP86tF2y5O1bnXFWdCVt55xyuYP%2BMwpIr2WZDe2%2B8%3D&am
> p;reserved=0>...
> Reading symbols from /home/davec/src/dpdk/build/app/testpmd...done.
> (gdb) b mlx5_set_min_inline
> Breakpoint 1 at 0x10579da0: file
> /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c, line 1251.
> (gdb) start
> Temporary breakpoint 2 at 0x100a9b38: file
> /home/davec/src/dpdk/app/test-pmd/testpmd.c, line 3234.
> Starting program: /home/davec/src/dpdk/build/app/testpmd -w
> 0000:01:00.0
> -w 0000:01:00.1 -l 4,8-79 -n 4 --iova-mode=pa -- --rxq=4 --rxd=1024
> --txq=4 --txd=4096 --nb-cores=8 -i -a --numa --forward-mode=rxonly
> warning: Could not load shared library symbols for linux-vdso64.so.1.
> Do you need "set solib-search-path" or "set sysroot"?
> warning: Unable to find libthread_db matching inferior's thread library,
> thread debugging will not be available.
> 
> Temporary breakpoint 2, main (argc=20, argv=0x7fffffffef68) at
> /home/davec/src/dpdk/app/test-pmd/testpmd.c:3234
> 3234		signal(SIGINT, signal_handler);
> Missing separate debuginfos, use: debuginfo-install
> advance-toolchain-at12.0-runtime-12.0-1.ppc64le
> libibverbs-17.2-3.el7.ppc64le
> (gdb) c
> Continuing.
> EAL: Detected 160 lcore(s)
> EAL: Detected 2 NUMA nodes
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'PA'
> EAL: No available hugepages reported in hugepages-2048kB
> EAL: Probing VFIO support...
> EAL: PCI device 0000:01:00.0 on NUMA socket 0
> EAL:   probe driver: 15b3:1019 net_mlx5
> net_mlx5: mlx5.c:1684: mlx5_dev_spawn(): MPLS over GRE/UDP tunnel
> offloading disabled due to old OFED/rdma-core version or firmware
> configuration
> 
> Breakpoint 1, mlx5_set_min_inline (spawn=0x7fffffffe500,
> config=0x7fffffffe4a0)
>      at /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c:1251
> 1251		if (config->txq_inline_min != MLX5_ARG_UNSET) {
> (gdb) p config->txq_inline_min
> $1 = -1
> (gdb) n
> 1268		if (config->hca_attr.eth_net_offloads) {
> (gdb) n
> 1323		switch (spawn->pci_dev->id.device_id) {
> (gdb) p/x spawn->pci_dev->id.vendor_id
> $2 = 0x15b3
> (gdb) p/x spawn->pci_dev->id.device_id
> $3 = 0x1019
> (gdb) n
> 1343			config->hw_vlan_insert = 0;
> (gdb) n
> 1344			config->txq_inline_min =
> MLX5_INLINE_HSIZE_NONE;
> (gdb) tbreak +1
> Temporary breakpoint 3 at 0x1057a0f0: file
> /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c, line 1345.
> (gdb) jump +1
> Continuing at 0x1057a0f0.
> 
> Temporary breakpoint 3, mlx5_set_min_inline (spawn=0x7fffffffe500,
> config=0x7fffffffe4a0)
>      at /home/davec/src/dpdk/drivers/net/mlx5/mlx5.c:1345
> 1345			break;
> (gdb) n
> 1351		DRV_LOG(DEBUG, "min tx inline configured: %d",
> config->txq_inline_min);
> (gdb) p config->txq_inline_min
> $4 = -1
> (gdb)
David Christensen Aug. 2, 2019, 6:17 p.m. UTC | #5 
>>> Yes, thank you for the patch, acked.
>>>
>>> The "txq_inline_min" value was not set for ConnectX-5 to 0 as default.
>>> At the TX queue setup time the "txq_inline_min" is checked against
>>> MLX5_ARG_UNSET and default value 0 is set:
>>>
>>> txq_set_params()
>>> ...
>>> inlen_mode = (config->txq_inline_min == MLX5_ARG_UNSET) ?
>>>                        0 : (unsigned int)config->txq_inline_min;
>>>
>>> So, there should be no negative backwards. Did you observe any?
>>
>> The gdb session below shows how the value is unchanged after passing
>> through mlx5_set_min_inline on my system without the change.
> 
> Yes, mlx5_set_min_inline() does not set txq_inline_min by default.
> So, your patch is OK.
> 
> I mean the actual inline_len is set to 0 before actual usage in txq_set_params()
> routine, so not setting config->txq_inline_min to default zero should not cause
> negative backwards.

So that implies the assert to verify that txq_inline_min >= 0 in 
mlx5_set_txlimit_params() is unnecessary.  Should the patch change and 
just remove the assert?

I think the patch is the more logical solution, setting the value at the 
source similar to what's done for all other adapters.

Dave
Slava Ovsiienko Aug. 4, 2019, 5:34 a.m. UTC | #6 
> -----Original Message-----
> From: David Christensen <drc@linux.vnet.ibm.com>
> Sent: Friday, August 2, 2019 21:18
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/mlx5: txq_inline_min not set for ConnectX-5
> adapters
> 
> >>> Yes, thank you for the patch, acked.
> >>>
> >>> The "txq_inline_min" value was not set for ConnectX-5 to 0 as default.
> >>> At the TX queue setup time the "txq_inline_min" is checked against
> >>> MLX5_ARG_UNSET and default value 0 is set:
> >>>
> >>> txq_set_params()
> >>> ...
> >>> inlen_mode = (config->txq_inline_min == MLX5_ARG_UNSET) ?
> >>>                        0 : (unsigned int)config->txq_inline_min;
> >>>
> >>> So, there should be no negative backwards. Did you observe any?
> >>
> >> The gdb session below shows how the value is unchanged after passing
> >> through mlx5_set_min_inline on my system without the change.
> >
> > Yes, mlx5_set_min_inline() does not set txq_inline_min by default.
> > So, your patch is OK.
> >
> > I mean the actual inline_len is set to 0 before actual usage in
> > txq_set_params() routine, so not setting config->txq_inline_min to
> > default zero should not cause negative backwards.
> 
> So that implies the assert to verify that txq_inline_min >= 0 in
> mlx5_set_txlimit_params() is unnecessary.  Should the patch change and just
> remove the assert?
I think no need to change the patch. If  I understand correctly this assert helped to
catch missing line in mlx5_set_min_inline() which your patch fixes.
If so - assert works well and should be kept, IMHO.

Also, please, delegate your patch to Raslan (http://patches.dpdk.org/project/dpdk/list/).

Thanks, Slava

> 
> I think the patch is the more logical solution, setting the value at the source
> similar to what's done for all other adapters.
> 
> Dave
Raslan Darawsheh Aug. 4, 2019, 10:53 a.m. UTC | #7 
Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Christensen
> Sent: Thursday, August 1, 2019 1:41 AM
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; David Christensen <drc@linux.vnet.ibm.com>
> Subject: [dpdk-dev] [PATCH] net/mlx5: txq_inline_min not set for ConnectX-
> 5 adapters
> 
> The function mlx5_set_min_inline() includes a switch() that checks
> various PCI device IDs in order to set the txq_inline_min value.  No
> value is set when the PCI device ID matches the ConnectX-5 adapters,
> resulting in an assert() failure later in the function
> mlx5_set_txlimit_params().
> 
> This error was encountered on an IBM Power 9 system running RHEL 7.6
> w/o Mellanox OFED installed.
> 
> Fixes: 38b4b397a57d ("net/mlx5: add Tx configuration and setup")
> Cc: viacheslavo@mellanox.com
> 
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
>  drivers/net/mlx5/mlx5.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 3a345c7..0f61dd5 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1341,6 +1341,7 @@ struct mlx5_dev_spawn_data {
>  		 * and PFC control may be broken, so disable feature.
>  		 */
>  		config->hw_vlan_insert = 0;
> +		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
>  		break;
>  	default:
>  		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
> --
> 1.8.3.1

Patch applied to next-net-mlx after fixing the wrong headline format

Kindest regards,
Raslan Darawsheh
diff mbox series

Patch

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 3a345c7..0f61dd5 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1341,6 +1341,7 @@  struct mlx5_dev_spawn_data {
 		 * and PFC control may be broken, so disable feature.
 		 */
 		config->hw_vlan_insert = 0;
+		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;
 		break;
 	default:
 		config->txq_inline_min = MLX5_INLINE_HSIZE_NONE;