Re: [PATCH v6 net-next 01/15] selftest: net: Deflake Periodic GC test in test_neigh.sh.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 4

- [Medium] The new completion barrier is the wrong signal:
  neigh_periodic_work() bumps NEIGH_CACHE_STAT_INC(tbl,…
- [Low] Undisclosed netns switch of the periodic_gc_runs read: the new
  get_periodic_gc_runs() helper queries the counter inside $ns1 (`ip -n…
- [Low] The patch fixes a currently reproducible selftest flake
  (extern_valid_common()'s periodic-GC wait in…
- [Low] Comment/code drift: the section header comment in
  extern_valid_common() still describes the removed sleep-based…
- [Low] The first `slowwait_for_counter` call (line 309) is the setup
  barrier that guarantees neigh_periodic_work() completed once and re-…

--

selftest: net: Deflake Periodic GC test in test_neigh.sh.

This replaces the two fixed sleeps in the periodic garbage collection test
case of test_neigh.sh with slowwait_for_counter() polling of the
periodic_gc_runs table statistic. A new helper, get_periodic_gc_runs(),
reads the counter, and the timeouts now account for the randomized initial
tbl->parms.reachable_time set up by neigh_table_init().

No Link: tags are present in the commit header.

[Severity: Low]
Would a Fixes: tag be appropriate here?  The commit message describes a
failure that is reproducible on current kernels ("The initial value is
randomly chosen, so if it is larger than 27s (larger than two "sleep"s),
the test fails"), and git blame points at the two commits that added the
sleeps being removed:

  171f2ee31a42f ("selftests: net: add test_neigh.sh") added the
  "sleep 10" wait

  f8fded7536a93 added the
  "sleep $(((orig_base_reachable / 1000) / 2 + 2))" wait

Without a tag the deflake will not be routed to trees that still run the
flaky version of the test.

> diff --git a/tools/testing/selftests/net/test_neigh.sh b/tools/testing/selftests/net/test_neigh.sh
> index 7c594bf6ead0..447423713266 100755
> --- a/tools/testing/selftests/net/test_neigh.sh
> +++ b/tools/testing/selftests/net/test_neigh.sh
> @@ -63,6 +63,15 @@ exit_cleanup_all()
>  	exit "${EXIT_STATUS}"
>  }
>  
> +get_periodic_gc_runs()
> +{
> +	local ns=$1
> +	local tbl_name=$2
> +
> +	ip -n "$ns" -j -s ntable show name "$tbl_name" | \
> +		jq '.[] | select(has("periodic_gc_runs")) | .["periodic_gc_runs"]'
> +}
> +

[Severity: Low]
The old code read periodic_gc_runs in the initial namespace:

  periodic_gc_runs_t0=$(ip -j -s ntable show name "$tbl_name" | jq ...)

while get_periodic_gc_runs() is called with "$ns1", so the counter is now
read inside ns1.  Was the namespace change intentional?  It is not
mentioned in the commit message.

At this commit it makes no difference, since neightbl_dump_info() in
net/core/neighbour.c emits neightbl_fill_info() (including NDTA_STATS) for
every registered table regardless of the requesting netns, and only the
per-device parms rows are filtered by net_eq().

Inside this series it does matter for a while, though.  After "neighbour:
Namespacify neigh_tables." the counter read in ns1 belongs to ns1's table
(default base_reachable 30000, so a 15s GC interval), while
base_reachable=10000 is still written to init_net's table below, so the
second wait's 10s budget is shorter than the interval ns1 actually uses.
The mismatch only disappears with the later "selftest: net: Specify netns
for ip ntable in test_neigh.sh." patch, which moves the ntable reads and
writes into ns1.  Would it be simpler to switch the ntable read/write and
the counter read in the same patch?

>  ################################################################################
>  # Tests
>  
> @@ -282,24 +291,30 @@ extern_valid_common()
>  	local periodic_gc_runs_t0
>  	local periodic_gc_runs_t1
>  	local orig_base_reachable
> +	local base_reachable=10000
>  	local orig_gc_stale
> +	local timeout
>  
>  	run_cmd "ip -n $ns1 neigh flush dev veth0"
>  	orig_thresh1=$(ip -j ntable show name "$tbl_name" | jq '.[] | select(has("thresh1")) | .["thresh1"]')
>  	orig_base_reachable=$(ip -j ntable show name "$tbl_name" | jq '.[] | select(has("thresh1")) | .["base_reachable"]')
> -	run_cmd "ip ntable change name $tbl_name thresh1 10 base_reachable 10000"
> +	run_cmd "ip ntable change name $tbl_name thresh1 10 base_reachable $base_reachable"
>  	orig_gc_stale=$(ip -n "$ns1" -j ntable show name "$tbl_name" dev veth0 | jq '.[]["gc_stale"]')
>  	run_cmd "ip -n $ns1 ntable change name $tbl_name dev veth0 gc_stale 1000"
>  	run_cmd "ip -n $ns1 neigh add $ip_addr lladdr $mac nud stale dev veth0 extern_valid"
>  	run_cmd "ip -n $ns1 neigh add ${subnet}3 lladdr $mac nud stale dev veth0"
> -	# Wait orig_base_reachable/2 for the new interval to take effect.
> -	run_cmd "sleep $(((orig_base_reachable / 1000) / 2 + 2))"
> +	# Wait up to neigh_set_reach_time(BASE_REACHABLE_TIME) (~45s) + 5s
> +	# for the new interval to take effect, see neigh_table_init().
> +	timeout=$(((orig_base_reachable / 1000) * 3 / 2 + 5))
> +	slowwait_for_counter "$timeout" 1 get_periodic_gc_runs "$ns1" "$tbl_name" >/dev/null

[Severity: Low]
Should the exit status of this first wait be checked?  Only stdout is
redirected, the status is never fed to check_err, and the test body runs
with set +e (set -e is enabled and cleared inside setup()).

slowwait_for_counter() in tools/testing/selftests/net/lib.sh does return
non-zero on timeout:

  slowwait_for_counter()
  {
  	local timeout=$1; shift
  	local delta=$1; shift

  	local base=$("$@")
  	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
  }

If this wait times out (up to ~50s with the default base_reachable=30000),
the script still adds the 20 entries, samples t0 and then waits only
base_reachable/1000/2 + 5 = 10s, a bound that assumes the new 5s interval
is already armed by the requeue at the end of neigh_periodic_work():

  queue_delayed_work(system_power_efficient_wq, &tbl->gc_work,
                     NEIGH_VAR(&tbl->parms, BASE_REACHABLE_TIME) >> 1);

The resulting failure is then reported as "Periodic garbage collection did
not run" with no hint that the setup wait was the thing that timed out.

There is also the inverse case: this wait can be satisfied by the initial
run armed in neigh_table_init() with the randomized reachable_time, so the
"new interval took effect" precondition is not really established.

>  	for i in {1..20}; do
>  		run_cmd "ip -n $ns1 neigh add ${subnet}$((i + 4)) nud none dev veth0"
>  	done
> -	periodic_gc_runs_t0=$(ip -j -s ntable show name "$tbl_name" | jq '.[] | select(has("periodic_gc_runs")) | .["periodic_gc_runs"]')
> -	run_cmd "sleep 10"
> -	periodic_gc_runs_t1=$(ip -j -s ntable show name "$tbl_name" | jq '.[] | select(has("periodic_gc_runs")) | .["periodic_gc_runs"]')
> +	periodic_gc_runs_t0=$(get_periodic_gc_runs "$ns1" "$tbl_name")
> +	# Wait up to BASE_REACHABLE_TIME / 2 + 5s, see neigh_periodic_work().
> +	timeout=$(((base_reachable / 1000) / 2 + 5))
> +	slowwait_for_counter "$timeout" 1 get_periodic_gc_runs "$ns1" "$tbl_name" >/dev/null
> +	periodic_gc_runs_t1=$(get_periodic_gc_runs "$ns1" "$tbl_name")

[Severity: Medium]
Is the periodic_gc_runs increment a strong enough barrier for the checks
that follow?  neigh_periodic_work() bumps the counter as its first
statement, before the table lock is taken and before anything is reaped:

  net/core/neighbour.c:neigh_periodic_work() {
  	NEIGH_CACHE_STAT_INC(tbl, periodic_gc_runs);

  	spin_lock_bh(&tbl->lock);
  	...
  }

and the bucket walk drops the lock and reschedules after every bucket:

  		spin_unlock_bh(&tbl->lock);
  		cond_resched();
  		spin_lock_bh(&tbl->lock);

slowwait_for_counter() returns as soon as the counter reaches base + 1, so
the pass may still be in flight when the script proceeds to:

  run_cmd "ip -n $ns1 neigh get ${subnet}3 dev veth0"
  check_fail $? "Entry without \"extern_valid\" flag survived periodic garbage collection"

Can this fail spuriously if the worker is preempted mid-scan, for instance
on a single-vCPU or debug kernel, given that only a couple of ip/jq
invocations separate the wait from the check?  The removed "sleep 10"
implicitly covered the whole pass.

The positive check just above it looks weakened in the same way: the
extern_valid entry can be reported as surviving simply because GC has not
walked its bucket yet.

Would polling the observable effect (the ${subnet}3 entry being gone), or
waiting for the counter to advance by 2, be more robust?

Related to the second timeout: base_reachable/1000/2 + 5 is measured from
the moment the increment is observed, but the next run is only armed at the
end of the pass, which eats into the 5s of slack.

[Severity: Low]
Does the block comment above this test case still describe what the code
does?  It reads:

  # Check that an "extern_valid" entry survives a periodic garbage
  # collection. Add an "extern_valid" entry, add more than "thresh1"
  # regular entries, wait "base_reachable" (longer than "gc_stale")
  # seconds and check that the "extern_valid" entry was not deleted.

The code no longer waits base_reachable seconds; it polls periodic_gc_runs
and returns on the first additional GC run, bounded by
base_reachable/1000/2 + 5.  The comment also survives unchanged to the end
of the series.  Could it be updated along with the wait, including the
"longer than gc_stale" rationale?

>  	[[ $periodic_gc_runs_t1 -ne $periodic_gc_runs_t0 ]]
>  	check_err $? "Periodic garbage collection did not run"
>  	run_cmd "ip -n $ns1 neigh get $ip_addr dev veth0 | grep \"extern_valid\""

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902203722.926528-1-kuniyu%40google.com