[PATCH net 0/3] devlink: region snapshot locking fix and selftest adjustments

[PATCH net 0/3] devlink: region snapshot locking fix and selftest adjustments

[Jakub Kicinski]

Minor fix for region snapshot locking and adjustments to selftests.

Jakub Kicinski (3):
  devlink: hold region lock when flushing snapshots
  selftests: devlink: fix the fd redirect in dummy_reporter_test
  selftests: devlink: add a warning for interfaces coming up

 net/core/devlink.c                                  |  2 ++
 .../selftests/drivers/net/netdevsim/devlink.sh      |  4 ++--
 .../selftests/drivers/net/netdevsim/devlink_trap.sh | 13 +++++++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.38.1

[PATCH net 1/3] devlink: hold region lock when flushing snapshots

[Jakub Kicinski]

Netdevsim triggers a splat on reload, when it destroys regions
with snapshots pending:

  WARNING: CPU: 1 PID: 787 at net/core/devlink.c:6291 devlink_region_snapshot_del+0x12e/0x140
  CPU: 1 PID: 787 Comm: devlink Not tainted 6.1.0-07460-g7ae9888d6e1c #580
  RIP: 0010:devlink_region_snapshot_del+0x12e/0x140
  Call Trace:
   <TASK>
   devl_region_destroy+0x70/0x140
   nsim_dev_reload_down+0x2f/0x60 [netdevsim]
   devlink_reload+0x1f7/0x360
   devlink_nl_cmd_reload+0x6ce/0x860
   genl_family_rcv_msg_doit.isra.0+0x145/0x1c0

This is the locking assert in devlink_region_snapshot_del(),
we're supposed to be holding the region->snapshot_lock here.

Fixes: 2dec18ad826f ("net: devlink: remove region snapshots list dependency on devlink->lock")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jiri@nvidia.com
---
 net/core/devlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6004bd0ccee4..d2df30829083 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -11925,8 +11925,10 @@ void devl_region_destroy(struct devlink_region *region)
 	devl_assert_locked(devlink);
 
 	/* Free all snapshots of region */
+	mutex_lock(&region->snapshot_lock);
 	list_for_each_entry_safe(snapshot, ts, &region->snapshot_list, list)
 		devlink_region_snapshot_del(region, snapshot);
+	mutex_unlock(&region->snapshot_lock);
 
 	list_del(&region->list);
 	mutex_destroy(&region->snapshot_lock);
-- 
2.38.1

Re: [PATCH net 1/3] devlink: hold region lock when flushing snapshots

[Jiri Pirko]

Thu, Dec 15, 2022 at 03:01:00AM CET, kuba@kernel.org wrote:
>Netdevsim triggers a splat on reload, when it destroys regions
>with snapshots pending:
>
>  WARNING: CPU: 1 PID: 787 at net/core/devlink.c:6291 devlink_region_snapshot_del+0x12e/0x140
>  CPU: 1 PID: 787 Comm: devlink Not tainted 6.1.0-07460-g7ae9888d6e1c #580
>  RIP: 0010:devlink_region_snapshot_del+0x12e/0x140
>  Call Trace:
>   <TASK>
>   devl_region_destroy+0x70/0x140
>   nsim_dev_reload_down+0x2f/0x60 [netdevsim]
>   devlink_reload+0x1f7/0x360
>   devlink_nl_cmd_reload+0x6ce/0x860
>   genl_family_rcv_msg_doit.isra.0+0x145/0x1c0
>
>This is the locking assert in devlink_region_snapshot_del(),
>we're supposed to be holding the region->snapshot_lock here.
>
>Fixes: 2dec18ad826f ("net: devlink: remove region snapshots list dependency on devlink->lock")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Ooup.

Thanks for the fix.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH net 2/3] selftests: devlink: fix the fd redirect in dummy_reporter_test

[Jakub Kicinski]

$number + > bash means redirect FD $number, e.g. commonly
used 2> redirects stderr (fd 2). The test uses 8192> to
write the number 8192 to a file, this results in:

  ./devlink.sh: line 499: 8192: Bad file descriptor

Oddly the test also papers over this issue by checking
for failure (expecting an error rather than success)
so it passes, anyway.

Fixes: ff18176ad806 ("selftests: Add a test of large binary to devlink health test")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/netdevsim/devlink.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index 9de1d123f4f5..a08c02abde12 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -496,8 +496,8 @@ dummy_reporter_test()
 
 	check_reporter_info dummy healthy 3 3 10 true
 
-	echo 8192> $DEBUGFS_DIR/health/binary_len
-	check_fail $? "Failed set dummy reporter binary len to 8192"
+	echo 8192 > $DEBUGFS_DIR/health/binary_len
+	check_err $? "Failed set dummy reporter binary len to 8192"
 
 	local dump=$(devlink health dump show $DL_HANDLE reporter dummy -j)
 	check_err $? "Failed show dump of dummy reporter"
-- 
2.38.1

[PATCH net 3/3] selftests: devlink: add a warning for interfaces coming up

[Jakub Kicinski]

NetworkManager (and other daemons) may bring the interface up
and cause failures in quiescence checks. Print a helpful warning,
and take the interface down again.

I seem to forget about this every time I run these tests on a new VM.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/netdevsim/devlink_trap.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
index 109900c817be..b64d98ca0df7 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
@@ -47,6 +47,17 @@ if [ -d "${NETDEVSIM_PATH}/devices/netdevsim${DEV_ADDR}" ]; then
 	exit 1
 fi
 
+check_netdev_down()
+{
+	state=$(cat /sys/class/net/${NETDEV}/flags)
+
+	if [ $((state & 1)) -ne 0 ]; then
+		echo "WARNING: unexpected interface UP, disable NetworkManager?"
+
+		ip link set dev $NETDEV down
+	fi
+}
+
 init_test()
 {
 	RET=0
@@ -151,6 +162,7 @@ trap_stats_test()
 
 	RET=0
 
+	check_netdev_down
 	for trap_name in $(devlink_traps_get); do
 		devlink_trap_stats_idle_test $trap_name
 		check_err $? "Stats of trap $trap_name not idle when netdev down"
@@ -254,6 +266,7 @@ trap_group_stats_test()
 
 	RET=0
 
+	check_netdev_down
 	for group_name in $(devlink_trap_groups_get); do
 		devlink_trap_group_stats_idle_test $group_name
 		check_err $? "Stats of trap group $group_name not idle when netdev down"
-- 
2.38.1

Re: [PATCH net 0/3] devlink: region snapshot locking fix and selftest adjustments

[Jacob Keller]

On 12/14/2022 6:00 PM, Jakub Kicinski wrote:
> Minor fix for region snapshot locking and adjustments to selftests.
> 
> Jakub Kicinski (3):
>    devlink: hold region lock when flushing snapshots
>    selftests: devlink: fix the fd redirect in dummy_reporter_test
>    selftests: devlink: add a warning for interfaces coming up
> 
>   net/core/devlink.c                                  |  2 ++
>   .../selftests/drivers/net/netdevsim/devlink.sh      |  4 ++--
>   .../selftests/drivers/net/netdevsim/devlink_trap.sh | 13 +++++++++++++
>   3 files changed, 17 insertions(+), 2 deletions(-)
> 

Whole series makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net 0/3] devlink: region snapshot locking fix and selftest adjustments

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 14 Dec 2022 18:00:59 -0800 you wrote:
> Minor fix for region snapshot locking and adjustments to selftests.
> 
> Jakub Kicinski (3):
>   devlink: hold region lock when flushing snapshots
>   selftests: devlink: fix the fd redirect in dummy_reporter_test
>   selftests: devlink: add a warning for interfaces coming up
> 
> [...]

Here is the summary with links:
  - [net,1/3] devlink: hold region lock when flushing snapshots
    https://git.kernel.org/netdev/net/c/b4cafb3d2c74
  - [net,2/3] selftests: devlink: fix the fd redirect in dummy_reporter_test
    https://git.kernel.org/netdev/net/c/2fc60e2ff972
  - [net,3/3] selftests: devlink: add a warning for interfaces coming up
    https://git.kernel.org/netdev/net/c/d1c4a3469e73

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html