Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Fri, 1 Dec 2023 20:21:34 +0200 (EET)
From:   Julian Anastasov <ja@ssi.bg>
To:     Doug Anderson <dianders@chromium.org>
cc:     Eric Dumazet <edumazet@google.com>,
        Judy Hsiao <judyhsiao@chromium.org>,
        David Ahern <dsahern@kernel.org>,
        Simon Horman <horms@kernel.org>,
        Brian Haley <haleyb.dev@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Joel Granados <joel.granados@gmail.com>,
        Leon Romanovsky <leon@kernel.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH v1] neighbour: Don't let neigh_forced_gc() disable
 preemption for long
In-Reply-To: <CAD=FV=UxS9qxYNdd+kqtW3VRSK=0H9ZPgW=CeSEjfbJXut+PaQ@mail.gmail.com>
Message-ID: <c4d90b06-a941-b275-38a9-2a891485ca4d@ssi.bg>
References: <20231201083926.1817394-1-judyhsiao@chromium.org> <CANn89iJMbMZdnJRP0CUVfEi20whhShBfO+DAmdaerhiXfiTx5A@mail.gmail.com> <CAD=FV=Vf18TxUWpGTN9b=iECq=5BmEoopQjsMH2U6bDX2=T3cQ@mail.gmail.com> <CANn89iLzmKOGhMeUUxeM=1b2PP3kieTeYsmpfA0GvJdcQMkgtQ@mail.gmail.com>
 <CAD=FV=UxS9qxYNdd+kqtW3VRSK=0H9ZPgW=CeSEjfbJXut+PaQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


	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@ssi.bg>