Re: [PATCH v1] neighbour: Don't let neigh_forced_gc() disable preemption for long

[Julian Anastasov]

	Hello,

On Fri, 1 Dec 2023, Doug Anderson wrote:

> The place we hit this wasn't actually with fuzzers but with normal
> usage in our labs. The only case where it was a really big problem was
> when neigh_forced_gc() was scheduled on a "little" CPU (in a
> big.LITTLE system) and that little CPU happened to be running at the
> lowest CPU frequency. Specifically Judy was testing on sc7180-trogdor
> and the lowest CPU Frequency of the "little" CPUs was 300 MHz. Since
> the littles are less powerful than the bigs, this is roughly the
> equivalent processing power of a big core running at 120 MHz.

	If we are talking about 32-bit systems with high HZ value I now 
see a little problem in neigh_alloc() where we may start neigh_forced_gc() 
later on gc_thresh3, not early on gc_thresh2 as expected. This can happen 
after a long idle period when last_flush becomes invalid and 'now'
is 'time_before' last_flush:

-	time_after(now, tbl->last_flush + 5 * HZ))) {
+	!time_in_range_open(now, tbl->last_flush, tbl->last_flush + 5 * HZ))) {

	With a big gap between gc_thresh2 and gc_thresh3 we
may work on large gc_list if we react on gc_thresh3 instead of
gc_thresh2. But such storms can not happen more than once per jiffie wrap.

> FWIW, we are apparently no longer seeing the bad latency after
> <https://crrev.com/c/4914309>, which does this:
> 
> # Increase kernel neighbor table size.
> echo 1024 > /proc/sys/net/ipv4/neigh/default/gc_thresh1
> echo 4096 > /proc/sys/net/ipv4/neigh/default/gc_thresh2

	We expect to keep the entries not much above 4096 and
we should see small gc_list.

> echo 8192 > /proc/sys/net/ipv4/neigh/default/gc_thresh3

	On invalid last_flush we will free 8192-4096 => 4096 entries
under BH lock.

Regards

--
Julian Anastasov <ja@xxxxxx>