* [PATCH 0/1] pyverbs: fix speed_to_str(), to handle disabled links
@ 2019-12-21 1:32 John Hubbard
2019-12-21 1:32 ` [PATCH 1/1] " John Hubbard
2019-12-21 10:03 ` [PATCH 0/1] " Leon Romanovsky
0 siblings, 2 replies; 6+ messages in thread
From: John Hubbard @ 2019-12-21 1:32 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Noa Osherovich, linux-rdma, John Hubbard
Hi,
This came up when I was running rdma-core tests on a two-machine setup,
where each card had two ports, but there was only one cable. So only
one port on each end was connected.
The main thing I expect to be up for debate is, what string to return
for speed, when a port is disabled or down? I initially thought about
returning '(Disabled/down)', but it seems more accurate to just report
'0.0 Gbps', so that's what I settled on.
Background: here's what I wrote when discussing this over on linux-mm
with Leon [1]:
It looks like this test suite assumes that every link is connected!
(Probably in most test systems, they are.) But in my setup, the ConnectX
cards each have two slots, and I only have (and only need) one cable. So
one link is up, and the other is disabled.
This leads to the other problem, which is that if a link is disabled,
the test suite finds a "0" token for attr.active_speed. That token is
not in the approved list, and so d.speed_to_str() asserts.
With some diagnostics added, I can see it checking each link: one
passes, and the other asserts:
diff --git a/tests/test_device.py b/tests/test_device.py
index 524e0e89..7b33d7db 100644
--- a/tests/test_device.py
+++ b/tests/test_device.py
@@ -110,6 +110,12 @@ class DeviceTest(unittest.TestCase):
assert 'Invalid' not in d.translate_mtu(attr.max_mtu)
assert 'Invalid' not in d.translate_mtu(attr.active_mtu)
assert 'Invalid' not in d.width_to_str(attr.active_width)
+ print("")
+ print('Diagnostics ===========================================')
+ print('phys_state: ', d.phys_state_to_str(attr.phys_state))
+ print('active_width): ', d.width_to_str(attr.active_width))
+ print('active_speed: ', d.speed_to_str(attr.active_speed))
+ print('END of Diagnostics ====================================')
assert 'Invalid' not in d.speed_to_str(attr.active_speed)
assert 'Invalid' not in d.translate_link_layer(attr.link_layer)
assert attr.max_msg_sz > 0x1000
assert attr.max_msg_sz > 0x1000
...and the test run from that is:
# ./build/bin/run_tests.py --verbose tests.test_device.DeviceTest
test_dev_list (tests.test_device.DeviceTest) ... ok
test_open_dev (tests.test_device.DeviceTest) ... ok
test_query_device (tests.test_device.DeviceTest) ... ok
test_query_device_ex (tests.test_device.DeviceTest) ... ok
test_query_gid (tests.test_device.DeviceTest) ... ok
test_query_port (tests.test_device.DeviceTest) ...
Diagnostics ===========================================
phys_state: Link up (5)
active_width): 4X (2)
active_speed: 25.0 Gbps (32)
END of Diagnostics ====================================
Diagnostics ===========================================
phys_state: Disabled (3)
active_width): 4X (2)
active_speed: Invalid speed
END of Diagnostics ====================================
FAIL
test_query_port_bad_flow (tests.test_device.DeviceTest) ... ok
======================================================================
FAIL: test_query_port (tests.test_device.DeviceTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/kernel_work/rdma-core/tests/test_device.py", line 135, in test_query_port
self.verify_port_attr(port_attr)
File "/kernel_work/rdma-core/tests/test_device.py", line 119, in verify_port_attr
assert 'Invalid' not in d.speed_to_str(attr.active_speed)
AssertionError
----------------------------------------------------------------------
Ran 7 tests in 0.055s
FAILED (failures=1)
[1] https://lore.kernel.org/r/b70ac328-2dc0-efe3-05c2-3e040b662256@nvidia.com
John Hubbard (1):
pyverbs: fix speed_to_str(), to handle disabled links
pyverbs/device.pyx | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 1/1] pyverbs: fix speed_to_str(), to handle disabled links
2019-12-21 1:32 [PATCH 0/1] pyverbs: fix speed_to_str(), to handle disabled links John Hubbard
@ 2019-12-21 1:32 ` John Hubbard
2019-12-23 14:39 ` Noa Osherovich
2019-12-26 9:12 ` Leon Romanovsky
2019-12-21 10:03 ` [PATCH 0/1] " Leon Romanovsky
1 sibling, 2 replies; 6+ messages in thread
From: John Hubbard @ 2019-12-21 1:32 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Noa Osherovich, linux-rdma, John Hubbard
For disabled links, the raw speed token is 0. However, speed_to_str()
doesn't have that in the list. This leads to an assertion when running
tests (test_query_port) when one link is down and other link(s) are up.
Fix this by returning '0.0 Gbps' for the zero speed case.
Cc: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
pyverbs/device.pyx | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyverbs/device.pyx b/pyverbs/device.pyx
index 33d133fd..cf7b75de 100755
--- a/pyverbs/device.pyx
+++ b/pyverbs/device.pyx
@@ -923,8 +923,8 @@ def width_to_str(width):
def speed_to_str(speed):
- l = {1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps', 8: '10.0 Gbps',
- 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
+ l = {0: '0.0 Gbps', 1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps',
+ 8: '10.0 Gbps', 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
try:
return '{s} ({n})'.format(s=l[speed], n=speed)
except KeyError:
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] pyverbs: fix speed_to_str(), to handle disabled links
2019-12-21 1:32 ` [PATCH 1/1] " John Hubbard
@ 2019-12-23 14:39 ` Noa Osherovich
2019-12-25 1:40 ` John Hubbard
2019-12-26 9:12 ` Leon Romanovsky
1 sibling, 1 reply; 6+ messages in thread
From: Noa Osherovich @ 2019-12-23 14:39 UTC (permalink / raw)
To: John Hubbard, Leon Romanovsky; +Cc: Jason Gunthorpe, linux-rdma
Hi John
On 12/21/2019 3:32 AM, John Hubbard wrote:
> For disabled links, the raw speed token is 0. However, speed_to_str()
> doesn't have that in the list. This leads to an assertion when running
> tests (test_query_port) when one link is down and other link(s) are up.
>
> Fix this by returning '0.0 Gbps' for the zero speed case.
>
> Cc: Noa Osherovich <noaos@mellanox.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> pyverbs/device.pyx | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/pyverbs/device.pyx b/pyverbs/device.pyx
> index 33d133fd..cf7b75de 100755
> --- a/pyverbs/device.pyx
> +++ b/pyverbs/device.pyx
> @@ -923,8 +923,8 @@ def width_to_str(width):
>
>
> def speed_to_str(speed):
> - l = {1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps', 8: '10.0 Gbps',
> - 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
> + l = {0: '0.0 Gbps', 1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps',
> + 8: '10.0 Gbps', 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
> try:
> return '{s} ({n})'.format(s=l[speed], n=speed)
> except KeyError:
This seems OK to me. BTW, what's the reported active_width for disabled links?
Maybe width_to_str could use a similar fix.
Thanks,
Noa
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] pyverbs: fix speed_to_str(), to handle disabled links
2019-12-23 14:39 ` Noa Osherovich
@ 2019-12-25 1:40 ` John Hubbard
0 siblings, 0 replies; 6+ messages in thread
From: John Hubbard @ 2019-12-25 1:40 UTC (permalink / raw)
To: Noa Osherovich, Leon Romanovsky; +Cc: Jason Gunthorpe, linux-rdma
On 12/23/19 6:39 AM, Noa Osherovich wrote:
...
>> diff --git a/pyverbs/device.pyx b/pyverbs/device.pyx
>> index 33d133fd..cf7b75de 100755
>> --- a/pyverbs/device.pyx
>> +++ b/pyverbs/device.pyx
>> @@ -923,8 +923,8 @@ def width_to_str(width):
>>
>>
>> def speed_to_str(speed):
>> - l = {1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps', 8: '10.0 Gbps',
>> - 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
>> + l = {0: '0.0 Gbps', 1: '2.5 Gbps', 2: '5.0 Gbps', 4: '5.0 Gbps',
>> + 8: '10.0 Gbps', 16: '14.0 Gbps', 32: '25.0 Gbps', 64: '50.0 Gbps'}
>> try:
>> return '{s} ({n})'.format(s=l[speed], n=speed)
>> except KeyError:
>
> This seems OK to me. BTW, what's the reported active_width for disabled links?
> Maybe width_to_str could use a similar fix.
>
Thanks for reviewing this! The reported active_width for disabled links on my
systems is showing up the same as for active links ("4X" in this case). So I don't
think we need a fix for width_to_str().
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] pyverbs: fix speed_to_str(), to handle disabled links
2019-12-21 1:32 ` [PATCH 1/1] " John Hubbard
2019-12-23 14:39 ` Noa Osherovich
@ 2019-12-26 9:12 ` Leon Romanovsky
1 sibling, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2019-12-26 9:12 UTC (permalink / raw)
To: John Hubbard; +Cc: Jason Gunthorpe, Noa Osherovich, linux-rdma
On Fri, Dec 20, 2019 at 05:32:56PM -0800, John Hubbard wrote:
> For disabled links, the raw speed token is 0. However, speed_to_str()
> doesn't have that in the list. This leads to an assertion when running
> tests (test_query_port) when one link is down and other link(s) are up.
>
> Fix this by returning '0.0 Gbps' for the zero speed case.
>
> Cc: Noa Osherovich <noaos@mellanox.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> pyverbs/device.pyx | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Thanks, applied
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] pyverbs: fix speed_to_str(), to handle disabled links
2019-12-21 1:32 [PATCH 0/1] pyverbs: fix speed_to_str(), to handle disabled links John Hubbard
2019-12-21 1:32 ` [PATCH 1/1] " John Hubbard
@ 2019-12-21 10:03 ` Leon Romanovsky
1 sibling, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2019-12-21 10:03 UTC (permalink / raw)
To: John Hubbard; +Cc: Jason Gunthorpe, Noa Osherovich, linux-rdma
On Fri, Dec 20, 2019 at 05:32:55PM -0800, John Hubbard wrote:
> Hi,
>
> This came up when I was running rdma-core tests on a two-machine setup,
> where each card had two ports, but there was only one cable. So only
> one port on each end was connected.
>
> The main thing I expect to be up for debate is, what string to return
> for speed, when a port is disabled or down? I initially thought about
> returning '(Disabled/down)', but it seems more accurate to just report
> '0.0 Gbps', so that's what I settled on.
>
> Background: here's what I wrote when discussing this over on linux-mm
> with Leon [1]:
>
> It looks like this test suite assumes that every link is connected!
> (Probably in most test systems, they are.)
I don't remember whenever the expectation of connection is by design or outcome
of mine and Jason's setups, where our cards are being connected in loopback mode
(port 0 to port 1 of the same card).
The loopback mode simplifies our kernel testing and development.
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-26 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-21 1:32 [PATCH 0/1] pyverbs: fix speed_to_str(), to handle disabled links John Hubbard
2019-12-21 1:32 ` [PATCH 1/1] " John Hubbard
2019-12-23 14:39 ` Noa Osherovich
2019-12-25 1:40 ` John Hubbard
2019-12-26 9:12 ` Leon Romanovsky
2019-12-21 10:03 ` [PATCH 0/1] " Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.